bump CPI to version 1.22.3 #1118
Conversation
nicolehanjing
force-pushed
the
topic/nicoleh/bump-cpi
branch
from
53214d4
to
a8585aa
Compare
|
/test |
|
Cluster Generation A/B Results: |
|
Cluster Generation A/B Results: |
nicolehanjing
force-pushed
the
topic/nicoleh/bump-cpi
branch
from
a8585aa
to
1ac84cb
Compare
|
Cluster Generation A/B Results: |
nicolehanjing
force-pushed
the
topic/nicoleh/bump-cpi
branch
from
1ac84cb
to
8ceb621
Compare
|
Cluster Generation A/B Results: |
nicolehanjing
force-pushed
the
topic/nicoleh/bump-cpi
branch
2 times, most recently
from
5464e76
to
02a8b4f
Compare
|
Cluster Generation A/B Results: |
|
Cluster Generation A/B Results: |
|
/test |
Tests failed! Build no: 1029 |
| metadata: | ||
| labels: | ||
| #@overlay/match missing_ok=True | ||
| vsphere-cpi/data-values-hash: #@ sha256.sum(yaml.encode(values)) |
There was a problem hiding this comment.
Could we move this to an annotation?
There was a problem hiding this comment.
@vijaykatam thanks for the review!
We noticed that CSI also uses label: https://github.com/vmware-tanzu/community-edition/blob/main/addons/packages/vsphere-csi/2.4.0/bundle/config/overlays/update-csi-driver.yaml#L77
Is it okay to just use the label? As we are close to FC, if this needs to be changed, can we update it in the next version?
There was a problem hiding this comment.
The issue with sha256 value is it can exceed 63 characters(restriction for label values) so you need to trim it and also cast to string to account for the case where truncated string is all numeric and k8s throws an error complaining integer overflow. Also labels means k8s will index the label. CSI needs to be changed but since this is net new I would encourage to not add tech debt.
There was a problem hiding this comment.
just want to double check - to update this, we will need to first update in tce repo, and then sync in framework repo and tkg-packages repo, right?
There was a problem hiding this comment.
Yep, the CPI daemonset will fail with the current code(due to label value validation) so a change would have been required either ways.
Also acknowledge the pain for upstream and downstream, we are looking to remove CRS for CPI in a future release so you won't need to sync to framework repo. For tce repo and tkg-packages, we will investigate a cayman mirror or other alternatives.
There was a problem hiding this comment.
Hi the sha256 will give out 256 bit not characters. which is 32 characters and we should be fine if we put it inside label
There was a problem hiding this comment.
Ok it's 64 characters in generated by ytt. so annotation should cover it, no need to truncate the sha
There was a problem hiding this comment.
In general, sha256 only generates 64 characters length string, and the length constraint for for label's value is 253. So there is no need to change.
https://kubernetes.io/docs/concepts/overview/working-with-objects/labels/#syntax-and-character-set @vijaykatam
There was a problem hiding this comment.
Valid label value:
must be 63 characters or less (can be empty),
sha265 ytt function returns more so needs to be trucated
There was a problem hiding this comment.
Oh yes you are right so the key prefix could be 253, not the value, I got it wrong. And no length limitation for annotation value
nicolehanjing
force-pushed
the
topic/nicoleh/bump-cpi
branch
from
02a8b4f
to
dcc6e38
Compare
|
Cluster Generation A/B Results: |
nicolehanjing
force-pushed
the
topic/nicoleh/bump-cpi
branch
from
dcc6e38
to
d7f61b3
Compare
|
Cluster Generation A/B Results: |
|
@vijaykatam can you help to review this PR? It's passing all the pipelines |
|
Just FYI, this change should only target 1.22 TKr. |
What this PR does / why we need it
bump CPI to version 1.22.3
Which issue(s) this PR fixes
Fixes #
Describe testing done for PR
generating CPI configs for data values
Release note
PR Checklist
Additional information
Special notes for your reviewer