HIVE-2391: vsphere zonal#2851
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: 2uasimojo The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
@2uasimojo: This pull request references HIVE-2391 which is a valid jira issue. DetailsIn response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
29af27f to
5249404
Compare
|
@jianping-shu this passed e2e-vsphere, so I reckon it's probably ready for you to take another stab at it! |
|
/hold Looks like I missed refactoring the preflight auth check for the new creds shape. |
5249404 to
39cf13e
Compare
|
/hold cancel |
|
The new multi-creds changes LGTM. My only concern (as noted in review comments) is that there are some additional fields (at least |
39cf13e to
896e851
Compare
|
/hold for QE |
896e851 to
7996e22
Compare
|
/test e2e security e2e: infra flake |
|
Allowable to override |
7996e22 to
066a4e4
Compare
afded4e to
870fdd5
Compare
|
Rebase only. Fixes pending. |
870fdd5 to
d3042d8
Compare
|
Okay, fixes are in. Thanks for the review @dlom |
|
regression test in progress... |
|
/test e2e-vsphere |
That's merged. Getting real close here. Currently just waiting on:
|
|
@dlom I've finished the round of regression tests.
I think we have 2 alternatives: |
d3042d8 to
3eafe07
Compare
Looks like I "fixed" that here. This isn't the same as the json unmarshaling issue we were seeing previously: this is about the spelling of the key in the Secret, which is definitely case-sensitive. We just need to decide which spelling we want to use and make it consistent. (Later) So my plan was to look at the installer's incarnations of this string and use whichever spelling they favor. Alas, it is not consistent there either -- there are cases of However, the singular (deprecated in install-config platform, but still viable in metadata) is always spelled TL;DR: I'll update the doc to match the code. |
Okay, that PR is merged, so I'll just rebase this one... |
3eafe07 to
dd75c19
Compare
|
@dlom Given that Jianping already validated using |
Followon addressing review from openshift#2731. MachinePool: - Removed `Topology` override - Restored ResourcePool and TagIDs overrides - Removed `osImage` detected from arbitrary master; using whatever's passed through from FD Topology (which defaults sanely if unset). Deprovision: - Changed `--vsphere-vcenter` StringVar to `--vsphere-vcenters` StringSliceVar Platform Creds: - Redesigned to take `vcenters`, a list of vcenter server/username/password, matching (and unmarshaling into) the corresponding chunk of metadata.json. Docs: - Updated install-config sample to zonal shape. - Documented new creds shape.
dd75c19 to
5e671cb
Compare
|
/test e2e-vsphere |
|
/test e2e-vsphere ...since openshift/release#77981 /test periodic-images (flake) /override ci/prow/security Known, to be dealt with elsewhere. |
|
@2uasimojo: Overrode contexts on behalf of 2uasimojo: ci/prow/security DetailsIn response to this:
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. |
|
/test e2e-vsphere Gonna hope this was a temporary DNS flake... |
|
/assign @suhanime |
|
/hold cancel This should be ready to go now. |
|
/test e2e-vsphere Trying to confirm this test infra fix: we want to see a successful run that uses |
|
@2uasimojo: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions 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. |
| for i := range platform.VCenters { | ||
| // Allow a hybrid input style: creds can be included in the json blob OR individually | ||
| // via env vars. The former takes precedence. | ||
| for i := range platform.VCenters { |
There was a problem hiding this comment.
this i shadows the top level loop's i, is that intentional?
There was a problem hiding this comment.
and they loop over the same thing
| return nil, fmt.Errorf("must provide --vsphere-vcenter or set %s env var", constants.VSphereVCenterEnvVar) | ||
| vcenterCredsb, err := json.Marshal(vcenterCreds) | ||
| if err != nil { | ||
| return nil, errors.Wrap(err, "failed to marhsal vcenter creds list") |
|
@2uasimojo the shadowed |
Co-Authored-By: @dlom
Summary by CodeRabbit
New Features
Deprecations
Documentation