OSASINFRA-3181 - Volume Types for OpenStack CPMS#7300
OSASINFRA-3181 - Volume Types for OpenStack CPMS#7300openshift-merge-robot merged 1 commit intoopenshift:masterfrom
Conversation
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
|
/cc pierreprinetti |
EmilienM
changed the title
EmilienM
force-pushed
the
osp_vol_type_cpms
branch
from
b30f1a4 to
924f1d9
Compare
openshift-merge-robot
added
the
needs-rebase
EmilienM
force-pushed
the
osp_vol_type_cpms
branch
from
924f1d9 to
4ba698f
Compare
openshift-merge-robot
removed
the
needs-rebase
pierreprinetti
left a comment
pierreprinetti
left a comment
There was a problem hiding this comment.
I know this is a WIP; I hope these comments are useful in this phase.
EmilienM
force-pushed
the
osp_vol_type_cpms
branch
from
4ba698f to
7c03015
Compare
EmilienM
force-pushed
the
osp_vol_type_cpms
branch
4 times, most recently
from
524abb1 to
213d480
Compare
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
|
I've removed the WIP so the CI can start provide feedback after some good progress. /hold until we have all we need to get it merged. /test e2e-openstack-kuryr |
openshift-ci
bot
added
the
do-not-merge/hold
|
/assign pierreprinetti |
EmilienM
force-pushed
the
osp_vol_type_cpms
branch
from
213d480 to
e09ccbc
Compare
|
/test e2e-agent-ha-dualstack |
r4f4
left a comment
r4f4
left a comment
There was a problem hiding this comment.
It would really help with reviewing if this PR was broken down into multiple commits.
EmilienM
force-pushed
the
osp_vol_type_cpms
branch
from
717b67c to
197dbdc
Compare
I agree with you that multiple commits could have help reviewers but at this point it'll be difficult to reproduce the iterations that we went through so if possible let's keep it like this. We'll do better the next time 👍 . |
Co-Authored-By: Emilien Macchi <emilien@redhat.com> Co-Authored-By: Pierre Prinetti <pierreprinetti@redhat.com>
EmilienM
force-pushed
the
osp_vol_type_cpms
branch
from
197dbdc to
a1ea1c8
Compare
|
/test e2e-agent-ha-dualstack e2e-aws-ovn e2e-openstack-ovn e2e-openstack-sdn-parallel openstack-manifests |
|
@EmilienM: The
The following commands are available to trigger optional jobs:
Use
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/test-infra repository. |
|
/test e2e-agent-ha-dualstack e2e-aws-ovn e2e-openstack-ovn e2e-openstack-sdn-parallel openstack-manifests |
|
/test okd-e2e-aws-ovn-upgrade okd-images |
|
/cc patrickdillon Thanks all |
|
/retest-required |
|
The following jobs are unrelated to that patch and can safely be ignored:
|
|
|
||
| // Types is the list of the volume types of the root volumes. | ||
| // This is mutually exclusive with Type. | ||
| // +required |
There was a problem hiding this comment.
@EmilienM @pierreprinetti can this new field be required and maintain backward compatibility, particularly during unmarshalling of the install config? I see the deprecated OpenStack controlPlane with type in rootVolume unit test and that looks good, but does that same scenario work when represented in a yaml install config? I would have suspected we would need to make this field optional/omitempty.
There was a problem hiding this comment.
to confirm that it works:
- I've done openstack: deploy ctlplane with rootVolume too release#41386 so
e2e-openstack-kuryris now re-running with a rootVolume that uses the deprecatedtypeparameter (and nottypes); if that works it's a proof that it's 100% backward compatible. I'll report the results to confirm that it worked and that the volumes were created with the right type. - I've tested it manually and it worked as expected, when only using
typein the install-config. The volumes were created with the unique type and no error was shown.
There was a problem hiding this comment.
I think it's working because of the conversion code, which always sets the Types field:
installer/pkg/types/conversion/installconfig.go
Lines 171 to 198 in a1ea1c8
There was a problem hiding this comment.
LGTM. This is my own ignorance of this detail of how golang unmarshaling works. Thanks!
There was a problem hiding this comment.
The job failed later for another reason but the install-config was well converted:
There was a problem hiding this comment.
I'm now seeing this error in another PR:
https://gcsweb-ci.apps.ci.l2s4.p1.openshiftapps.com/gcs/origin-ci-test/pr-logs/pull/openshift_release/41450/rehearse-41450-periodic-ci-shiftstack-shiftstack-ci-main-periodic-4.14-e2e-openstack-kuryr/1681836643008385024/artifacts/e2e-openstack-kuryr/ipi-install-install/build-log.txt
But I suspect it's because the step registry doesn't have the latest installer, whereas the previous step registry which generated the install-config and converted it, has the latest, since we can see the new parameter:
https://gcsweb-ci.apps.ci.l2s4.p1.openshiftapps.com/gcs/origin-ci-test/pr-logs/pull/openshift_release/41450/rehearse-41450-periodic-ci-shiftstack-shiftstack-ci-main-periodic-4.14-e2e-openstack-kuryr/1681836643008385024/artifacts/e2e-openstack-kuryr/openstack-conf-installconfig/artifacts/install-config.yaml
There was a problem hiding this comment.
I'll keep an eye on that, but again both CI and manual tests confirmed that it's supposed to work...
There was a problem hiding this comment.
Testing again with pull-ci-openshift-installer-master-e2e-openstack-kuryr via openshift/release#41450 which might gives a more recent build for the installer.
|
@EmilienM: The
The following commands are available to trigger optional jobs:
Use
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/test-infra repository. |
|
/test e2e-openstack-kuryr |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: patrickdillon 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 |
openshift-ci
bot
added
the
approved
|
@EmilienM: The following tests failed, say
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/test-infra repository. I understand the commands that are listed here. |
| * `rootVolume` (optional object): Defines the root volume for instances in the machine pool. The instances use ephemeral disks if not set. | ||
| * `size` (required integer): Size of the root volume in GB. Must be set to at least 25. | ||
| * `type` (required string): The volume pool to create the volume from. | ||
| * `type` (optional string): The volume pool to create the volume from. It was replaced by `types`. |
There was a problem hiding this comment.
Should have been
| * `type` (optional string): The volume pool to create the volume from. It was replaced by `types`. | |
| * `type` (deprecated string): The volume pool to create the volume from. It was replaced by `types`. |
| if poolPlatform := pool.Platform.Name(); poolPlatform != openstack.Name { | ||
| return nil, fmt.Errorf("non-OpenStack machine-pool: %q", poolPlatform) | ||
| } | ||
| if pool.Replicas == nil || *pool.Replicas < 1 { |
There was a problem hiding this comment.
I believe this change broke the NFV jobs, where we set the number of worker nodes to 0.
https://prow.ci.openshift.org/?job=*4.14-e2e-openstack*nfv*
We used to generate machinesets per zone, even if the number of replicas was 0, and we no longer do that. This causes the workerSpecs to be empty, and consequently workerServerGroupNames.
We may want to either revert these couple of lines of code where we return early in case of no replicas, or provide a default value for openstack_worker_server_group_names in terraform.
... even if zero workers is requested from install-config (replicas == 0). This was a non backward compatible change introduced by openshift#7300
6 participants
typesparameter