Skip to content

Prepend the namespace to ironic nodenames to avoid conflicts#797

Merged
metal3-io-bot merged 1 commit intometal3-io:masterfrom
asalkeld:ironic-host-namespaced
Mar 30, 2021
Merged

Prepend the namespace to ironic nodenames to avoid conflicts#797
metal3-io-bot merged 1 commit intometal3-io:masterfrom
asalkeld:ironic-host-namespaced

Conversation

@asalkeld
Copy link
Copy Markdown
Contributor

@asalkeld asalkeld commented Feb 16, 2021 •
edited
Loading

Modify the hostname for ironic to prepend the namespace to the name to avoid naming conflicts.

@metal3-io-bot metal3-io-bot requested a review from imain February 16, 2021 00:18
@asalkeld asalkeld marked this pull request as draft February 16, 2021 00:18
@metal3-io-bot
Copy link
Copy Markdown
Contributor

metal3-io-bot commented Feb 16, 2021

Hi @asalkeld. Thanks for your PR.

I'm waiting for a metal3-io member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@metal3-io-bot metal3-io-bot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Feb 16, 2021
@asalkeld asalkeld marked this pull request as ready for review February 16, 2021 05:00
@metal3-io-bot metal3-io-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 16, 2021
@asalkeld
Copy link
Copy Markdown
Contributor Author

asalkeld commented Feb 16, 2021

/ok-to-test

@metal3-io-bot
Copy link
Copy Markdown
Contributor

metal3-io-bot commented Feb 16, 2021

@asalkeld: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

Details

In response to this:

/ok-to-test

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

hardys
hardys reviewed Feb 16, 2021
View reviewed changes
Comment thread pkg/provisioner/ironic/ironic.go Outdated
@hardys hardys requested review from dhellmann and sadasu February 16, 2021 09:13
@hardys
Copy link
Copy Markdown
Member

hardys commented Feb 16, 2021

/cc @maelk PTAL - this changes the naming convention for hosts in Ironic, to avoid potential conflict in a multi-namespace deployment

@metal3-io-bot metal3-io-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Feb 16, 2021
dhellmann
dhellmann reviewed Feb 17, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@dhellmann dhellmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a little unfortunate that we have to leak the deployment configuration into the provisioner, but I don't see an elegant way to avoid it that doesn't leak the implementation of the provisioner up into main().

Comment thread pkg/provisioner/ironic/ironic.go Outdated
Comment thread config/manager/manager.yaml
@asalkeld asalkeld mentioned this pull request Feb 18, 2021
Merged
maelk
maelk reviewed Feb 19, 2021
View reviewed changes
Comment thread pkg/provisioner/ironic/ironic.go Outdated
andfasano
andfasano reviewed Feb 22, 2021
View reviewed changes
Comment thread pkg/provisioner/ironic/ironic.go Outdated
@hardys hardys mentioned this pull request Feb 22, 2021
Merged
andfasano
andfasano reviewed Feb 24, 2021
View reviewed changes
Comment thread pkg/provisioner/ironic/ironic.go Outdated
sadasu
sadasu reviewed Mar 2, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@sadasu sadasu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@metal3-io-bot
Copy link
Copy Markdown
Contributor

metal3-io-bot commented Mar 2, 2021

@sadasu: adding LGTM is restricted to approvers and reviewers in OWNERS files.

Details

In response to this:

/lgtm

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

sadasu
sadasu reviewed Mar 4, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@sadasu sadasu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have to consider the disable case, where BMO goes from watching all namespaces back to just watching 1 namespace? In that case would it be better for the method that generates the node name to always generate the name with the Namespace prefix?

Comment thread pkg/provisioner/ironic/ironic.go Outdated
@sadasu
Copy link
Copy Markdown
Member

sadasu commented Mar 4, 2021

/test-integration

imain
imain approved these changes Mar 8, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@imain imain left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me

@stbenjam
Copy link
Copy Markdown
Member

stbenjam commented Mar 8, 2021

/ok-to-test

@metal3-io-bot metal3-io-bot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Mar 8, 2021
zaneb
zaneb reviewed Mar 15, 2021
View reviewed changes
Comment thread pkg/provisioner/ironic/ironic.go Outdated
Comment thread pkg/provisioner/ironic/ironic.go Outdated
Comment thread pkg/provisioner/ironic/ironic.go Outdated
@asalkeld
Copy link
Copy Markdown
Contributor Author

asalkeld commented Mar 16, 2021

/test-integration

2 similar comments
@asalkeld
Copy link
Copy Markdown
Contributor Author

asalkeld commented Mar 16, 2021

/test-integration

@kirankt
Copy link
Copy Markdown
Contributor

kirankt commented Mar 16, 2021

/test-integration

@furkatgofurov7
Copy link
Copy Markdown
Member

furkatgofurov7 commented Mar 16, 2021

There was an issue with our CI recently, so tests did not run properly in the previous runs. Hopefully should be fine now.

/test-integration

@asalkeld asalkeld requested a review from zaneb March 18, 2021 03:02
zaneb
zaneb reviewed Mar 19, 2021
View reviewed changes
Comment thread pkg/provisioner/ironic/ironic.go Outdated
Comment thread main.go Outdated
Comment thread pkg/provisioner/ironic/ironic.go Outdated
zaneb
zaneb reviewed Mar 23, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@zaneb zaneb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One tiny comment.
/approve

Comment thread pkg/provisioner/ironic/ironic.go Outdated
@metal3-io-bot
Copy link
Copy Markdown
Contributor

metal3-io-bot commented Mar 23, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: asalkeld, imain, zaneb

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@metal3-io-bot metal3-io-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 23, 2021
@asalkeld asalkeld requested a review from zaneb March 23, 2021 22:21
@hardys
Copy link
Copy Markdown
Member

hardys commented Mar 24, 2021

/test-integration

2 similar comments
@zaneb
Copy link
Copy Markdown
Member

zaneb commented Mar 24, 2021

/test-integration

@hardys
Copy link
Copy Markdown
Member

hardys commented Mar 25, 2021

/test-integration

@zaneb
Copy link
Copy Markdown
Member

zaneb commented Mar 27, 2021

This has been around for a while and has had quite a few eyes on it.
/lgtm

@metal3-io-bot metal3-io-bot added lgtm Indicates that a PR is ready to be merged. and removed lgtm Indicates that a PR is ready to be merged. labels Mar 27, 2021
@asalkeld
Copy link
Copy Markdown
Contributor Author

asalkeld commented Mar 28, 2021

needed a unit test fix after @honza's PR got merged.

@asalkeld
Copy link
Copy Markdown
Contributor Author

asalkeld commented Mar 28, 2021

/retest

@asalkeld
Prepend the namespace to ironic nodenames to avoid conflicts
e9afe2a 
- Don't search by nodename if it has "~" in it
- if POD_NAMESPACE is "" then use watchNamespace for the leader
@zaneb
Copy link
Copy Markdown
Member

zaneb commented Mar 30, 2021

/test-integration
/lgtm

@metal3-io-bot metal3-io-bot added the lgtm Indicates that a PR is ready to be merged. label Mar 30, 2021
@metal3-io-bot metal3-io-bot merged commit b236263 into metal3-io:master Mar 30, 2021
@zaneb zaneb mentioned this pull request Mar 31, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dhellmann dhellmann dhellmann left review comments

@dtantsur dtantsur dtantsur left review comments

@hardys hardys hardys left review comments

@zaneb zaneb Awaiting requested review from zaneb

+4 more reviewers

@imain imain imain approved these changes

@sadasu sadasu sadasu left review comments

@maelk maelk maelk left review comments

@andfasano andfasano andfasano left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.