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.

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

[node-agent] Perform hosts probing on RegistryConfig.Hosts change #11008

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

dimitar-kostadinov
Copy link
Contributor

How to categorize this PR?

/area robustness
/kind enhancement

What this PR does / why we need it:
There are cases where RegistryConfig.Hosts is changed, e.g. CACerts is added or rotated, the mirror URL is updated, etc. Is such cases the hosts probing should be performed to ensure that new configuration is valid.

Which issue(s) this PR fixes:
Part of gardener-extension-registry-cache/issues/290

Special notes for your reviewer:
N/A

Release note:

`gardener-node-agent` now performs host probing when `RegistryConfig.Hosts` changes.

@gardener-prow gardener-prow bot added area/robustness Robustness, reliability, resilience related kind/enhancement Enhancement, improvement, extension size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Dec 10, 2024
@gardener-prow gardener-prow bot added cla: no Indicates the PR's author has not signed the cla-assistant.io CLA. cla: yes Indicates the PR's author has signed the cla-assistant.io CLA. and removed cla: no Indicates the PR's author has not signed the cla-assistant.io CLA. labels Dec 10, 2024
@ialidzhikov
Copy link
Member

/assign

Copy link
Contributor

@Kostov6 Kostov6 left a comment

Choose a reason for hiding this comment

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

Nit

@@ -315,3 +320,16 @@ func computeContainerdRegistryDiffs(newRegistries, oldRegistries []extensionsv1a

return r
}

func suppressHostsProbing(newRegistries, oldRegistries []extensionsv1alpha1.RegistryConfig) {
for i := 0; i < len(newRegistries); i++ {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
for i := 0; i < len(newRegistries); i++ {
for i := range newRegistries {

Copy link
Contributor

gardener-prow bot commented Dec 16, 2024

@dimitar-kostadinov: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
pull-gardener-e2e-kind-ha-single-zone 1c09b76 link true /test pull-gardener-e2e-kind-ha-single-zone
pull-gardener-e2e-kind-operator-seed 1c09b76 link true /test pull-gardener-e2e-kind-operator-seed
pull-gardener-e2e-kind-gardenadm 1c09b76 link true /test pull-gardener-e2e-kind-gardenadm

Full PR test history. Your PR dashboard. Command help for this repository.
Please help us cut down on flakes by linking this test failure to an open flake report or filing a new flake report if you can't find an existing one. Also see our testing guideline for how to avoid and hunt flakes.

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. I understand the commands that are listed here.

@gardener-prow gardener-prow bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 20, 2024
Copy link
Contributor

gardener-prow bot commented Dec 20, 2024

PR needs rebase.

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-ci-robot
Copy link
Contributor

The Gardener project currently lacks enough active contributors to adequately respond to all PRs.
This bot triages PRs according to the following rules:

  • After 15d of inactivity, lifecycle/stale is applied
  • After 15d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 7d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

  • Mark this PR as fresh with /remove-lifecycle stale
  • Mark this PR as rotten with /lifecycle rotten
  • Close this PR with /close

/lifecycle stale

@gardener-prow gardener-prow bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jan 4, 2025
@ialidzhikov
Copy link
Member

/remove-lifecycle stale

@gardener-prow gardener-prow bot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jan 6, 2025
Comment on lines +324 to +335
func suppressHostsProbing(newRegistries, oldRegistries []extensionsv1alpha1.RegistryConfig) {
for i := 0; i < len(newRegistries); i++ {
if ptr.Deref(newRegistries[i].ReadinessProbe, false) {
index := slices.IndexFunc(oldRegistries, func(config extensionsv1alpha1.RegistryConfig) bool {
return newRegistries[i].Upstream == config.Upstream
})
if index != -1 && reflect.DeepEqual(newRegistries[i].Hosts, oldRegistries[index].Hosts) {
newRegistries[i].ReadinessProbe = ptr.To(false)
}
}
}
}
Copy link
Member

Choose a reason for hiding this comment

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

There should be better ways to do achieve this.

Right now, you are modifying the readinessProbe field of the registry config fetched from the OSC. It looks like hack/anti-pattern to modify the desired state of your object.

Instead:

Copy link
Member

Choose a reason for hiding this comment

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

https://github.com/ialidzhikov/gardener/tree/enh/node_agent_osc_hosts_probing-poc: PoC for applying the registries similar to how files are applied (the 2nd bullet from #11008 (comment)).

Copy link
Contributor

gardener-prow bot commented Jan 14, 2025

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from ialidzhikov. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found 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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/robustness Robustness, reliability, resilience related cla: yes Indicates the PR's author has signed the cla-assistant.io CLA. kind/enhancement Enhancement, improvement, extension needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants