-
Notifications
You must be signed in to change notification settings - Fork 202
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.
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
Openshift machine api #188
Openshift machine api #188
Conversation
cmd/nodelink-controller/main.go
Outdated
@@ -61,15 +60,15 @@ const ( | |||
controllerName = "nodelink" | |||
|
|||
// machineAnnotationKey is the annotation storing a link between a node and it's machine. Should match upstream cluster-api machine controller. (node.go) | |||
machineAnnotationKey = "cluster.k8s.io/machine" | |||
machineAnnotationKey = "machine.k8s.io/machine" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems a little weird. I feel like we should either keep this the same as upstream, or go ahead and change it to openshift.io
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure, it was meant to be machine.openshift.io
, thanks!
@@ -61,6 +61,20 @@ spec: | |||
args: | |||
- --logtostderr=true | |||
- --v=3 | |||
- name: nodelink-controller-deprecated | |||
image: quay.io/coreos/machine-api-operator:origin-v4.0-2019-01-31-041134 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be nice to template these in after having read them from the config like we do for the others. Even if it's temporary, it would be nice for these to come from the release image. I suppose this is not a total blocker though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is never gonna come from the payload/release image. The ones coming from the payload are the ones built from master so adding support for the new types.
The payload CI build will replace every appearance of https://github.com/openshift/machine-api-operator/blob/master/install/image-references#L8 with the corresponding image Stream built by CI.
We could template in the configMap though but it wouldn't make any difference
With the obvious exception of the Godep mirror location for cluster-api, LGTM. |
e5ceeea
to
fcc0fb9
Compare
/test all |
/test e2e-aws-operator |
/test e2e-aws |
f87aec1
to
963f70a
Compare
/test e2e-aws-operator |
963f70a
to
3fb5ada
Compare
/test e2e-aws |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/approve
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: frobware, paulfantom The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
name = "sigs.k8s.io/cluster-api" | ||
branch = "master" | ||
name = "github.com/openshift/cluster-api" | ||
revision = "91fca585a85b163ddfd119fd09c128c9feadddca" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fine for now though let's use tags once we finish the mirroring
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Few nits left.
@@ -61,6 +61,20 @@ spec: | |||
args: | |||
- --logtostderr=true | |||
- --v=3 | |||
- name: nodelink-controller-deprecated |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 for naming
@@ -61,15 +61,15 @@ const ( | |||
controllerName = "nodelink" | |||
|
|||
// machineAnnotationKey is the annotation storing a link between a node and it's machine. Should match upstream cluster-api machine controller. (node.go) | |||
machineAnnotationKey = "cluster.k8s.io/machine" | |||
machineAnnotationKey = "machine.openshift.io/machine" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -25,7 +25,7 @@ import ( | |||
) | |||
|
|||
const ( | |||
machineAnnotationKey = "cluster.k8s.io/machine" | |||
machineAnnotationKey = "machine.openshift.io/machine" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, but that belongs to a different PR. We need to go step by step. Let's switch the api group. Then do improvements
We are transitioning the cluster API types from k8s.io over a machine.openshift.io group. https://github.com/openshift/cluster-api/tree/91fca585a85b163ddfd119fd09c128c9feadddca Although this give us a necessary level of autonomy today, we aim to consolidate back with upstream once there's a release of the API stable enough For reference: Backward compatible changes on machine api operator [openshift/machine-api-operator#185](openshift/machine-api-operator#185) [openshift/machine-api-operator#188](openshift/machine-api-operator#188) [openshift/machine-api-operator#191](openshift/machine-api-operator#191) Libvirt actuator support for new group openshift/cluster-api-provider-libvirt#111
We are transitioning the cluster API types from k8s.io over a machine.openshift.io group. https://github.com/openshift/cluster-api/tree/91fca585a85b163ddfd119fd09c128c9feadddca Although this give us a necessary level of autonomy today, we aim to consolidate back with upstream once there's a release of the API stable enough For reference: Backward compatible changes on machine api operator [openshift/machine-api-operator#185](openshift/machine-api-operator#185) [openshift/machine-api-operator#188](openshift/machine-api-operator#188) [openshift/machine-api-operator#191](openshift/machine-api-operator#191) Libvirt actuator support for new group openshift/cluster-api-provider-libvirt#111
We are transitioning the cluster API types from k8s.io over a machine.openshift.io group. https://github.com/openshift/cluster-api/tree/91fca585a85b163ddfd119fd09c128c9feadddca Although this give us a necessary level of autonomy today, we aim to consolidate back with upstream once there's a release of the API stable enough For reference: Backward compatible changes on machine api operator [openshift/machine-api-operator#185](openshift/machine-api-operator#185) [openshift/machine-api-operator#188](openshift/machine-api-operator#188) [openshift/machine-api-operator#191](openshift/machine-api-operator#191) Libvirt actuator support for new group openshift/cluster-api-provider-libvirt#111
…e-deletion Skip deletion of stop machines if there is an error
Since we are migrating to use openshift.io for machine API. We want to do a backward compatible transition.
This runs controllers for both groups simultaneously
TODO: