Skip to content

Support "none" cloudprovider#119

Merged
openshift-merge-robot merged 1 commit intoopenshift:masterfrom
vrutkovs:none-platform
Jan 9, 2019
Merged

Support "none" cloudprovider#119
openshift-merge-robot merged 1 commit intoopenshift:masterfrom
vrutkovs:none-platform

Conversation

@vrutkovs
Copy link
Contributor

@vrutkovs vrutkovs commented Jan 8, 2019

After openshift/installer#982 was merged BYOR installs use platform: none. Our install waits for all CVO operators to complete successfully, but kube-controller-manager-operator fails with this setup:

      - apiVersion: config.openshift.io/v1
        kind: ClusterOperator
        metadata:
          creationTimestamp: '2019-01-08T16:39:29Z'
          generation: 1
          name: openshift-kube-controller-manager-operator
          namespace: ''
          resourceVersion: '10227'
          selfLink: /apis/config.openshift.io/v1/clusteroperators/openshift-kube-controller-manager-operator
          uid: f80027da-1363-11e9-85a4-42010af00002
        spec: {}
        status:
          conditions:
          - lastTransitionTime: '2019-01-08T16:49:28Z'
            message: 'ConfigObservationFailing: configmap/cluster-config-v1.kube-system: no recognized cloud provider platform found: map[string]interface {}{"none":map[string]interface {}{}}'
            reason: ConfigObservationFailing
            status: 'True'
            type: Failing
          - lastTransitionTime: '2019-01-08T16:49:28Z'
            message: 3 of 3 nodes are at revision 1
            status: 'True'
            type: Available
          - lastTransitionTime: '2019-01-08T16:49:28Z'
            message: 3 of 3 nodes are at revision 1
            reason: AllNodesAtLatestRevision
            status: 'False'
            type: Progressing
          extension: null
          version: ''

This PR adds a support for this platform (as libvirt it doesn't modify extendedArguments) and updates tests to verify these three cases.

Don't know how to test that config was not modified, but I could extend the tests later on. Implemented

Verified that it unblocks platform: none installs

Fixes #100

@openshift-ci-robot openshift-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jan 8, 2019
@vrutkovs
Copy link
Contributor Author

vrutkovs commented Jan 8, 2019

/retest

1 similar comment
@vrutkovs
Copy link
Contributor Author

vrutkovs commented Jan 9, 2019

/retest

Copy link

@frobware frobware left a comment

Choose a reason for hiding this comment

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

LGTM, but wondering why the "libvirt" and "none" testcases are different than aws for the expected cloudprovider field.

hasExtendedArguments: true,
}, {
installConfig: "platform:\n libvirt: {}\n",
cloudProvider: "",
Copy link

Choose a reason for hiding this comment

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

Given the "aws" test case, how come this is not "libvirt"? Perhaps I didn't look closely enough but I see for libvirt we return return observerConfig, errs - curious as to why "aws" is more explicit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This var is not being checked, as the func would immediately return observedConfig so in test ObserveCloudProviderNames would return immediately.

Reworked the test:

  • cloudProvider is not initialized for none and libvirt
  • test struct has cloudProviderNum - expected number of cloudproviders
  • test now ensures expected and actual number of cloudproviders are equals

Copy link

Choose a reason for hiding this comment

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

I think cloudProviderNum is better expressed as cloudProviderCount. For me (at least) Num looks like an index.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, I was choosing between these two, renamed to Count

@frobware
Copy link

frobware commented Jan 9, 2019

/lgtm

@openshift-ci-robot openshift-ci-robot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jan 9, 2019
@frobware
Copy link

frobware commented Jan 9, 2019

/approve

@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: frobware, vrutkovs

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@vrutkovs
Copy link
Contributor Author

vrutkovs commented Jan 9, 2019

/retest

@vrutkovs
Copy link
Contributor Author

vrutkovs commented Jan 9, 2019

/test e2e-aws

case platform["libvirt"] != nil:
// this means we are using libvirt
return observedConfig, errs
case platform["none"] != nil:
Copy link
Member

Choose a reason for hiding this comment

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

Does the none need to be specifies explicitly? Just asking what is the difference when the cloud provider is not specified and when it is set to none.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

none is the platform name in the installer, not cloudprovider name

@openshift-merge-robot openshift-merge-robot merged commit b123832 into openshift:master Jan 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants