Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Label nodes with expected OperatingSystemConfig name #9757

Merged
merged 6 commits into from
May 23, 2024
Merged

Label nodes with expected OperatingSystemConfig name #9757

merged 6 commits into from
May 23, 2024

Conversation

MichaelEischer
Copy link
Contributor

@MichaelEischer MichaelEischer commented May 15, 2024 •
edited

How to categorize this PR?

/area control-plane
/kind enhancement

What this PR does / why we need it:

Label all workers with the name of their expected operating system config.

Nodes with an outdated operating system config key are skipped while checking whether a shoot is healthy. Currently, this requires botanist to know how to calculate the OSCkey based on a node object, which leaks implementation details. Instead, each node is now just labeled with the expected config key. This allows for future extensions of the OSCkey calculation without having to introduce additional labels on the node objects.

By including the KeyName in the operatingsystemconfig.Data struct, no other component has to know how the KeyName is calculated. The existing oscName field is not really suitable as it also includes a keySuffix which the external components shouldn't know about.

Which issue(s) this PR fixes:
Part of #9699 . For nodeToBeDeleted I've kept the fallback to nodeOSCKeyDifferentFromSecretOSCKey(node, secretOSCKey) for nodes that are not labeled yet. Otherwise the healthcheck would be inaccurate until the next successful reconcile of the Worker extension object.

Special notes for your reviewer:
Reworked implementation of #9465 .

Release note:

Nodes are now labeled with "worker.gardener.cloud/gardener-node-agent-secret-name", which includes the expected name of the secret used by gardener-node-agent.

@MichaelEischer
operatingsystemconfig: secret name is never nil
bf5342c 
The only place that creates operatingsystemconfig.Data objects always
sets SecretName to a pointer. Thus, convert the string pointer to a
plain string.
@MichaelEischer
label workers with their expected operating system config key
d7a8f69 
Nodes with an outdated operating system config key are skipped while
checking whether a shoot is healthy. Currently, this requires botanist
to know how to calculate the OSCkey based on a node object, which leaks
implementation details. Instead, each node is now just labeled with the
expected config key. This allows for future extensions of the OSCkey
calculation without having to introduce additional labels on the node
objects.

By including the keyName in the operatingsystemconfig.Data struct, no
other component has to know how the keyName is calculated.
@MichaelEischer
worker: enforce the OSC exists during deploy
10af74d
@MichaelEischer
test health check fallback for nodes without OSC key
ee6e966
@gardener-prow gardener-prow bot added area/control-plane Control plane related kind/enhancement Enhancement, improvement, extension labels May 15, 2024
@gardener-prow gardener-prow bot added cla: yes Indicates the PR's author has signed the cla-assistant.io CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels May 15, 2024
@gardener-prow Gardener Prow
Copy link
Contributor

gardener-prow bot commented May 15, 2024

Hi @MichaelEischer. Thanks for your PR.

I'm waiting for a gardener 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.

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-sigs/prow repository.

@gardener-prow gardener-prow bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label May 15, 2024
@rfranzke
Copy link
Member

/assign

rfranzke
rfranzke reviewed May 16, 2024
View reviewed changes
Copy link
Member

@rfranzke rfranzke left a comment

Choose a reason for hiding this comment

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

/ok-to-test

@gardener-prow gardener-prow 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 May 16, 2024
@MichaelEischer
Copy link
Contributor Author

/retest

@rfranzke
Copy link
Member

/assign

rfranzke
rfranzke requested changes May 21, 2024
View reviewed changes
Copy link
Member

@rfranzke rfranzke left a comment

Choose a reason for hiding this comment

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

Nice PR, thanks!

pkg/component/extensions/operatingsystemconfig/operatingsystemconfig.go Show resolved Hide resolved
pkg/apis/core/v1beta1/constants/types_constants.go Outdated Show resolved Hide resolved
pkg/component/extensions/operatingsystemconfig/operatingsystemconfig.go Outdated Show resolved Hide resolved
pkg/utils/gardener/shoot.go Outdated Show resolved Hide resolved
pkg/gardenlet/operation/botanist/worker.go Outdated Show resolved Hide resolved
pkg/gardenlet/operation/botanist/worker.go Outdated Show resolved Hide resolved
pkg/component/extensions/worker/worker.go Outdated Show resolved Hide resolved
pkg/gardenlet/operation/botanist/worker_test.go Outdated Show resolved Hide resolved
@MichaelEischer
Copy link
Contributor Author

I've addressed the review comments. PTAL

rfranzke
rfranzke reviewed May 23, 2024
View reviewed changes
Copy link
Member

@rfranzke rfranzke left a comment

Choose a reason for hiding this comment

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

Awesome @MichaelEischer
Let's go

/lgtm
/approve

@gardener-prow gardener-prow bot added the lgtm Indicates that a PR is ready to be merged. label May 23, 2024
@gardener-prow Gardener Prow
Copy link
Contributor

gardener-prow bot commented May 23, 2024

LGTM label has been added.

Git tree hash: e04bd5a18567d0980a6fd953bc6726a74dcaf87a

@gardener-prow Gardener Prow
Copy link
Contributor

gardener-prow bot commented May 23, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rfranzke

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

The pull request process is described here

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

@gardener-prow gardener-prow bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 23, 2024
@MichaelEischer
Copy link
Contributor Author

/retest

@gardener-prow gardener-prow bot merged commit db36cd7 into gardener:master May 23, 2024
18 checks passed
@MichaelEischer MichaelEischer deleted the label-nodes-with-osckey branch May 27, 2024 12:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rfranzke rfranzke rfranzke requested changes

@oliver-goetz oliver-goetz Awaiting requested review from oliver-goetz

@timebertt timebertt Awaiting requested review from timebertt

Assignees

@rfranzke rfranzke

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/control-plane Control plane related cla: yes Indicates the PR's author has signed the cla-assistant.io CLA. kind/enhancement Enhancement, improvement, extension 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.

None yet
2 participants
@MichaelEischer @rfranzke