Skip to content

Set bmh 'name' in metadata as hostname#842

Merged
metal3-io-bot merged 1 commit intometal3-io:masterfrom
Nordix:fix/default-node-name
Apr 1, 2021
Merged

Set bmh 'name' in metadata as hostname#842
metal3-io-bot merged 1 commit intometal3-io:masterfrom
Nordix:fix/default-node-name

Conversation

@kashifest
Copy link
Copy Markdown
Member

@kashifest kashifest commented Mar 31, 2021

This will set 'name' field of metaData as bmh name, otherwise by default ironic name gets set as hostname by default and ~ in hostname seems to be problematic for kubeadm during cloudinit nodeRegistration.

@metal3-io-bot metal3-io-bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Mar 31, 2021
@kashifest
Copy link
Copy Markdown
Member Author

/test-v1a3-integration

@hardys
Copy link
Copy Markdown
Member

hardys commented Mar 31, 2021

/cc @asalkeld @zaneb

@metal3-io-bot metal3-io-bot requested review from asalkeld and zaneb March 31, 2021 08:05
@kashifest
Copy link
Copy Markdown
Member Author

To be exact, this is the issue we are facing in v1a3 CI . We dont see it in normal integration (v1a4) since in that case the node name is taken from data template:

nodeRegistration.name: Invalid value: "metal3~node-1": a lowercase RFC 1123 subdomain must consist of lower case alphanumeric characters, '-' or '.', and must start and end with an alphanumeric character (e.g. 'example.com', regex used for validation is '[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*')

@kashifest
Copy link
Copy Markdown
Member Author

/test-integration
/test-v1a3-centos-integration
/test-centos-integration

@kashifest kashifest changed the title WIP: Change the nameseparator of nodename from ~ to - Change the nameseparator of nodename from ~ to - Mar 31, 2021
@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 Mar 31, 2021
Comment thread pkg/provisioner/ironic/ironic.go
@kashifest kashifest force-pushed the fix/default-node-name branch from c1db8ad to 09cb4b5 Compare March 31, 2021 09:51
@kashifest
Copy link
Copy Markdown
Member Author

/test-integration
/test-v1a3-integration

@kashifest
Copy link
Copy Markdown
Member Author

/test-v1a3-integration

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.

How is the ironic node name getting used as the hostname anyway? If we can find a way to stop that it would be preferable.

Comment thread pkg/provisioner/ironic/ironic.go Outdated
softPowerOff = "soft power off"
powerNone = "None"
nameSeparator = "~"
nameSeparator = "-"
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is unsafe; if we are going to change it, it must be to . not -.

@hardys
Copy link
Copy Markdown
Member

hardys commented Mar 31, 2021

How is the ironic node name getting used as the hostname anyway? If we can find a way to stop that it would be preferable.

I thought the same thing - in the metadata we're setting e.g p.host.ObjectMeta.Name which is the BMH name, not the name in Ironic, right? @dtantsur does Ironic inject the Ironic name in addition to the meta_data.json?

@hardys
Copy link
Copy Markdown
Member

hardys commented Mar 31, 2021

How is the ironic node name getting used as the hostname anyway? If we can find a way to stop that it would be preferable.

I thought the same thing - in the metadata we're setting e.g p.host.ObjectMeta.Name which is the BMH name, not the name in Ironic, right? @dtantsur does Ironic inject the Ironic name in addition to the meta_data.json?

The answer appears to be yes, so this name change isn't as "internal" as we originally envisaged - it seems Ironic injects name in addition to the keys we create in BMO, and presumably cloud-init uses that to set the hostname...

# cat /mnt/openstack/latest/meta_data.json  | jq .
{
  "local-hostname": "ostest-worker-2",
  "local_hostname": "ostest-worker-2",
  "metal3-name": "ostest-worker-2",
  "metal3-namespace": "openshift-machine-api",
  "uuid": "c0fd5a6b-2a7c-46c1-8000-d11b0dc0d8b8",
  "name": "openshift-machine-api~ostest-worker-2"
}

(this is from an openshift environment running the latest BMO)

@zaneb
Copy link
Copy Markdown
Member

zaneb commented Mar 31, 2021

The answer appears to be yes, so this name change isn't as "internal" as we originally envisaged - it seems Ironic injects name in addition to the keys we create in BMO, and presumably cloud-init uses that to set the hostname...

It's done using setdefault(), so if we set the name field explicitly in the metadata, that will resolve the problem.

This will set 'name' field of metaData as bmh name, otherwise by default ironic
name gets set as hostname by default and ~ in hostname seems to be problematic
for kubeadm during cloudinit nodeRegistration.
@kashifest kashifest force-pushed the fix/default-node-name branch from 09cb4b5 to 3994584 Compare March 31, 2021 19:03
@kashifest kashifest changed the title Change the nameseparator of nodename from ~ to - Set bmh 'name' in metadata as hostname Mar 31, 2021
@kashifest
Copy link
Copy Markdown
Member Author

/test-integration
/test-v1a3-integration

@zaneb
Copy link
Copy Markdown
Member

zaneb commented Mar 31, 2021

/lgtm

@metal3-io-bot metal3-io-bot added the lgtm Indicates that a PR is ready to be merged. label Mar 31, 2021
@kashifest
Copy link
Copy Markdown
Member Author

Thanks @zaneb that seems to work. Tests have passed now.

@kashifest
Copy link
Copy Markdown
Member Author

Thanks @hardys for your insight.

Copy link
Copy Markdown
Contributor

@asalkeld asalkeld left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

@metal3-io-bot
Copy link
Copy Markdown
Contributor

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

Details

In response to this:

/lgtm
/approve

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.

@maelk
Copy link
Copy Markdown
Member

maelk commented Apr 1, 2021

/approve

@metal3-io-bot
Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: asalkeld, kashifest, maelk

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 Apr 1, 2021
@metal3-io-bot metal3-io-bot merged commit dcae92d into metal3-io:master Apr 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

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. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants