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

Exclude backingStore.etcd's image from getting populated when disabled #2411

Open
wants to merge 1 commit into
base: v0.20
Choose a base branch
from

Conversation

ApsTomar
Copy link
Contributor

@ApsTomar ApsTomar commented Jan 17, 2025

What issue type does this pull request address? (keep at least one, remove the others)
/kind bugfix

What does this pull request do? Which issues does it resolve? (use resolves #<issue_number> if possible)
resolves ENG-5526 (#2398)

Please provide a short message that should be published in the vcluster release notes
Fixed an issue in vcluster v0.20 where the controlPlane.backingStore.etcd's image is getting populated with default values even when it has been disabled.

What else do we need to know?
While reproducing the issue, I came to a conclusion that this issue happens in vcluster version 0.20 when k8s version < 1.30, a quick fix for the user would be to either upgrade vcluster to latest version OR upgrade k8s to a version >= 1.30

Now, for the users on vcluster 0.20 and having k8s version < 1.30, this PR would resolve the issue. Please refer to the artifact attached below. With this fix, controlPlane.backingStore.etcd's image will not get populated if disabled.

One thing to note here- similar to etcd image, there are distro images as well present in the output as mentioned in the issue. These can also be fixed in the similar fashion as this PR. Let me know if we want to resolve that along with this etcd image fix.

Screenshot from 2025-01-17 21-22-43

@ApsTomar ApsTomar marked this pull request as draft January 17, 2025 16:04
@ApsTomar ApsTomar marked this pull request as ready for review January 20, 2025 14:29
Copy link
Contributor

@lizardruss lizardruss left a comment

Choose a reason for hiding this comment

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

Some logic updates needed.

// build values
if apiImage != "" {
vConfig.ControlPlane.Distro.K8S.Version = parseImage(apiImage).Tag
}
if etcdImage != "" {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you still need to set the image when the etcd deployment is enabled.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

With these changes, the behaviour is now consistent with the latest vcluster (v0.22) when etcd.Deploy is enabled. But I understand the concern, I will check again to be sure and update with more details.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lizardruss I have checked and verified about the scenario that you mentioned- when etcd deployment is enabled - in that case it's going to take the default image value present in default values.yaml, as expected.

The reason being, we first get the default helm values (as mentioned above), then we read the values.yaml file provided by the user and merge override only those values which are supplied by the user to create the vcluster config. So, etcd.Deploy will be set to true and the image value will be used from default values.

Below are the test artifacts supporting the above-mentioned facts-

  • Here is the values.yaml file (vc20.yaml) which sets etcd.Deploy.Enabled = true and the etcd pod with the expected image version 3.5.13:
    image

  • Here is the result of helm get values command:
    image

@ApsTomar ApsTomar force-pushed the issue-2398-fix-helm-values branch from aadcf7c to 634d741 Compare January 29, 2025 13:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants