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

feat(chart): bump snapshot-controller and snapshot-validation-webhook to v6.3.3 #5107

Merged
merged 3 commits into from
Feb 15, 2024
Merged

feat(chart): bump snapshot-controller and snapshot-validation-webhook to v6.3.3 #5107

merged 3 commits into from
Feb 15, 2024

Conversation

FrankYang0529
Copy link
Member

@FrankYang0529 FrankYang0529 commented Feb 5, 2024 •
edited

Test plan:

  1. Create a Harvester cluster.
  2. Check snapshot-controller and webhook is v6.3.3.

@FrankYang0529 FrankYang0529 force-pushed the bump-csi-snapshotter branch 3 times, most recently from 1e74516 to 8cd44dc Compare February 5, 2024 10:02
@FrankYang0529 FrankYang0529 changed the title feat(chart): bump snaoshot to v0.6.3 feat(chart): bump snaoshot to v6.3.2 Feb 5, 2024
@FrankYang0529 FrankYang0529 changed the title feat(chart): bump snaoshot to v6.3.2 feat(chart): bump snaoshot to v6.3.3 Feb 5, 2024
@FrankYang0529 FrankYang0529 changed the title feat(chart): bump snaoshot to v6.3.3 feat(chart): bump snaoshot-controller and snapshot-validation-webhook to v6.3.3 Feb 5, 2024
@FrankYang0529 FrankYang0529 force-pushed the bump-csi-snapshotter branch 6 times, most recently from b360426 to 51ec7cf Compare February 6, 2024 07:26
@FrankYang0529
Copy link
Member Author

FrankYang0529 commented Feb 6, 2024 •
edited

Hi @ibrokethecloud, could you also help me check this PR? I can use this branch to create a cluster. At first, I also got error like following in the managedchart.

failed calling webhook \"snapshot-validation-webhook.csi.kubernetes.io\": failed to call webhook: Post \"https://harvester-snapshot-validation-webhook.kube-system.svc:443/volumesnapshot?timeout=2s\": dial tcp 10.96.21.231:443: connect: connection refused"

The reason is that snapshot-validation-webhook gets error like #4920 (comment).

The error can be fixed when I removed all snapshot-validation-webhook pods at the same time (not deployment). I'm not sure why the first deployment can't work.

I follow the example like https://github.com/piraeusdatastore/helm-charts/tree/snapshot-validation-webhook-1.9.0/charts/snapshot-validation-webhook to change container port or add --port=8443 to the argument, but it can't fix the error.

Could you help to take a look? Thank you!

@FrankYang0529 FrankYang0529 force-pushed the bump-csi-snapshotter branch 2 times, most recently from 4a16b2c to 2e4d7d5 Compare February 6, 2024 10:14
@FrankYang0529
feat(chart): bump snapshot-controller and snapshot-validation-webhook…
5b111ca 
… to v6.3.3

Signed-off-by: PoAn Yang <poan.yang@suse.com>
@ibrokethecloud
Copy link
Contributor

there are 2 changes in the last commit:

  • drop volumesnapshotclasses from list of resources being watched for by the snapshot webhook. This currently breaks the installation of the helm chart during CI as the harvester chart also creates a volumesnapshotclass
  • renamed the deployment template file of snapshot webhook to webhook-deployment to overcome what seems to be the race condition, where the pods error out due tls error, likely because of order in which resources are created. deletion of snapshot webhook pods fixes the issue, likely because now a valid certificate exists. fleet creates resources based on name of file, and not like helm. [Bug] Fleet ignores order in file names and breaks with previous behavior  rancher/fleet#411. renaming the file works around the tls startup error as the cert is always created before the deployment.

@w13915984028 w13915984028 changed the title feat(chart): bump snaoshot-controller and snapshot-validation-webhook to v6.3.3 feat(chart): bump snapshot-controller and snapshot-validation-webhook to v6.3.3 Feb 12, 2024
w13915984028
w13915984028 approved these changes Feb 12, 2024
View reviewed changes
Copy link
Member

@w13915984028 w13915984028 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks.

@ibrokethecloud ibrokethecloud requested review from connorkuehl and tserong and removed request for WebberHuang1118 and ibrokethecloud February 14, 2024 05:45
@FrankYang0529
Copy link
Member Author

Test with last commit. The snapshot-controller and webhook is v6.3.3. Also, backup and restore can work. LGTM. Thanks.

tserong
tserong approved these changes Feb 15, 2024
View reviewed changes
Copy link

@tserong tserong left a comment

Choose a reason for hiding this comment

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

LGTM, I just have one comment/suggestion.

deploy/charts/harvester/dependency_charts/snapshot-validation-webhook/templates/deployment.yaml Outdated
@@ -27,7 +27,8 @@ spec:
{{- end }}
ports:
- name: https
containerPort: {{ .Values.service.port | default 443 }}
containerPort: {{ .Values.args.port | default 8443 }}
protocol: TCP
Copy link

Choose a reason for hiding this comment

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

You mentioned:

I follow the example like https://github.com/piraeusdatastore/helm-charts/tree/snapshot-validation-webhook-1.9.0/charts/snapshot-validation-webhook to change container port or add --port=8443 to the argument, but it can't fix the error.

Given Gaurav's subsequent fix, is this port change necessary? If not, I'd suggest to revert it (unless there's some other reason I'm not yet aware of to change the port)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I think this update is not helpful. Reverted it. Thanks.

deploy/charts/harvester/dependency_charts/snapshot-validation-webhook/values.yaml Outdated
pullPolicy: IfNotPresent

podAnnotations: {}

args:
tlsPrivateKeyFile: /etc/snapshot-validation-webhook/certs/tls.key
tlsCertFile: /etc/snapshot-validation-webhook/certs/tls.crt
port: 8443
Copy link

Choose a reason for hiding this comment

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

Same as my previous comment.

Copy link
Member Author

Choose a reason for hiding this comment

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

Reverted it. Thanks.

@FrankYang0529
revert(chart): change port from 8443 to 443
1975cff 
Signed-off-by: PoAn Yang <poan.yang@suse.com>
@ibrokethecloud ibrokethecloud merged commit 51e8e74 into harvester:master Feb 15, 2024
5 checks passed
@FrankYang0529 FrankYang0529 deleted the bump-csi-snapshotter branch February 16, 2024 01:23
@FrankYang0529
Copy link
Member Author

@mergify backport v1.2

@mergify Mergify
Copy link

mergify bot commented Apr 24, 2024 •
edited

backport v1.2

✅ Backports have been created

@mergify mergify bot mentioned this pull request Apr 24, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tserong tserong tserong approved these changes

@connorkuehl connorkuehl connorkuehl approved these changes

@w13915984028 w13915984028 w13915984028 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@FrankYang0529 @ibrokethecloud @tserong @connorkuehl @w13915984028