Drop AWS UPI control-plane Machines and compute MachineSets#1649
Drop AWS UPI control-plane Machines and compute MachineSets#1649openshift-merge-robot merged 5 commits intoopenshift:masterfrom
Conversation
There was a problem hiding this comment.
nit provide instructions of using their favorite editor to edit the specific file, and also example of how it would look.
There was a problem hiding this comment.
nit provide instructions of using their favorite editor to edit the specific file
What's their favorite editor, sed? ;)
and also example of how it would look.
If I make this a sed command, can I skip the example? I want to minimize the examples we have to update whenever we bump something in the install-config.
There was a problem hiding this comment.
For in repo docs I think sed is fine provided it's easy enough to follow and we feel it won't create a headache to maintain. For openshift-docs we need to have an example. So decide if it's easier to have them look approximately the same or not.
There was a problem hiding this comment.
Since this is the only line we want them to change in the install-config.yaml file in this flow, I vote for sed.
There was a problem hiding this comment.
Ok, if you're fine with sed i'm fine with it.
There was a problem hiding this comment.
I've pushed a sed approach with 5e34eb130 -> cc7d56dea (also rebases onto master). It's a bit fiddly, for reasons discussed in the cc7d56dea commit message. Is it worth it? I'm not sure what else we'd do for POSIX-compliant YAML edits, but we could always give an example in... Go? Python?... that makes a YAML-aware edit if we don't mind leaving folks without whatever tool we pick to translate themselves instead of copy/pasting.
84e5a80 to
5f7554b
Compare
5f7554b to
5e34eb1
Compare
|
There is a slightly cleaner way to do it with Fn:: style instead of using !
style. This is fine though.
I really dont like hiding the subnet semantics of the machine API. It will
simplify the doc now, but the VPC template is the one which will either be
ignored (existing customer VPC) or reused for multiple clusters. In both
cases, burrowing cluster specific names in at the VPC level will cause
harm. The current questions and headaches are all still there and will come
back as customer cases.
Steve
Mobile
…On Thu, Apr 18, 2019, 7:53 PM W. Trevor King ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In upi/aws/cloudformation/01_vpc.yaml
<#1649 (comment)>:
> @@ -245,6 +252,8 @@ Resources:
Tags:
- Key: Application
Value: !Ref "AWS::StackName"
+ - Key: Name
+ Value: !Join ["-", [!Ref InfrastructureName, "-private-", FIXME: zone?]]
Should this FIXME be...
I figured it out (5f7554b
<5f7554b>
-> 5e34eb1
<5e34eb1>).
But, ick ;).
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1649 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAB2SIFKYRRETNTFA66VDJDPRECZDANCNFSM4HG7IFFA>
.
|
cc7d56d to
0dbbc1f
Compare
|
cc7d56dea. -> 0dbbc1f80 gets us closer, but the cluster is still stuck on the authentication operator waiting for a route: $ oc get -o jsonpath='{.status.conditions[?(@.type == "Failing")].message}{"\n"}' clusteroperator authentication
Failing: error checking current version: unable to check route health: failed to GET route: EOFI'm tearing down to take another run... |
0dbbc1f to
aa5238f
Compare
Summarizing some already-brief out-of-band discussion, my main concern is with pushing broken manifests into the cluster. The three approaches to avoiding that are: a. Matching your environment to the installer's existing expectations (the route I'm taking here). My main concern with (c), and to a lesser extend with the currently-unavoidable (b), is that the |
aa5238f to
c94286b
Compare
|
Ok, this pass everything went through. So I've pulled the WIP tag off the subject and dropped the FIXME note with aa5238fd2 -> c94286bc9. Ready for another round of review :). |
There was a problem hiding this comment.
Should be rm -f openshift/99_openshift-cluster-api_master-*.yaml, or else would hit such error:
$ ./openshift-install create ignition-configs --dir ./upi_2019-04-24-23-57-58
FATAL failed to fetch Bootstrap Ignition Config: failed to load asset "Master Machines": master machine manifests are required if you also provide openshift/99_openshift-cluster-api_master-user-data-secret.yaml
There was a problem hiding this comment.
$ ./openshift-install create ignition-configs --dir ./upi_2019-04-24-23-57-58 FATAL failed to fetch Bootstrap Ignition Config: failed to load asset "Master Machines": master machine manifests are required if you also provide openshift/99_openshift-cluster-api_master-user-data-secret.yaml
What version installer are you using? We dropped that check in March.
There was a problem hiding this comment.
# ./openshift-install version ./openshift-install v4.1.0-201904211700-dirty built from commit f3b726cc151f5a3d66bc7e23e81b3013f1347a7e release image registry.svc.ci.openshift.org/ocp/release:4.1.0-rc.0
There was a problem hiding this comment.
That's a broken commit hash:
$ git show f3b726cc151f5a3d66bc7e23e81b3013f1347a7e
fatal: bad object f3b726cc151f5a3d66bc7e23e81b3013f1347a7eMaybe https://bugzilla.redhat.com/show_bug.cgi?id=1694183 . That makes it hard to know what code was included in your installer. Checking 4.1.0-rc.0:
$ oc adm release info --commits quay.io/openshift-release-dev/ocp-release:4.1.0-rc.0 | grep ' installer '
installer https://github.com/openshift/installer 49092083ce354636d63dd25140ba10e113b54452
That commit exists in our repository, and it's well after #1493 landed:
$ git log --first-parent --format='%ad %h %d %s' --date=iso -96 49092083ce354636d63dd25140ba10e113b54452 | tail -n1
2019-03-28 19:03:31 -0700 b4403e685 Merge pull request #1493 from abhinavdahiya/fix_machines_platformPulling the installer out of that release with a recent oc gives a nominal version that matches yours, which suggests that your buggy installer does include #1493:
$ oc version --client
Client Version: version.Info{Major:"4", Minor:"0+", GitVersion:"v4.0.0-alpha.0+c0c0569-2185", GitCommit:"c0c0569cfd", GitTreeState:"clean", BuildDate:"2019-04-25T19:48:12Z", GoVersion:"go1.10.3", Compiler:"gc", Platform:"linux/amd64"}
$ oc adm release extract --command openshift-install quay.io/openshift-release-dev/ocp-release:4.1.0-rc.0
$ sha256sum openshift-install
f8ee2f8a466fc551347b8fa8fad897a08a408fe85ca7ebab97dcb08fe6e1ce73 openshift-install
$ ./openshift-install version
./openshift-install v4.1.0-201904211700-dirty
built from commit f3b726cc151f5a3d66bc7e23e81b3013f1347a7e
release image quay.io/openshift-release-dev/ocp-release@sha256:345ec9351ecc1d78c16cf0853fe0ef2d9f48dd493da5fdffc18fa18f45707867Pulling the ART-build tarball from the mirrors gives the same version information:
$ wget https://mirror.openshift.com/pub/openshift-v4/clients/ocp/4.1.0-rc.0/openshift-install-linux-4.1.0-rc.0.tar.gz
$ sha256sum openshift-install-linux-4.1.0-rc.0.tar.gz
b187f874dbfd81a378fe83bba11b81882d50f2855dd6c6bb2d9c4a7724708009 openshift-install-linux-4.1.0-rc.0.tar.gz
$ tar xvf openshift-install-linux-4.1.0-rc.0.tar.gz
openshift-install
$ sha256sum openshift-install
f8ee2f8a466fc551347b8fa8fad897a08a408fe85ca7ebab97dcb08fe6e1ce73 openshift-install
$ ./openshift-install version
./openshift-install v4.1.0-201904211700-dirty
built from commit f3b726cc151f5a3d66bc7e23e81b3013f1347a7e
release image quay.io/openshift-release-dev/ocp-release@sha256:345ec9351ecc1d78c16cf0853fe0ef2d9f48dd493da5fdffc18fa18f45707867so at least we're consistent. It's not clear to me why your version output includes release image registry.svc.ci.openshift.org/ocp/release:4.1.0-rc.0; oc adm release extract is supposed to be pinning that by digest, not by a potentially-floating tag. But I don't see your string in this installer:
$ strings openshift-install | grep 'required if you also provide'
...no hits...And I cannot reproduce your error:
$ mkdir testing
$ cat <<EOF >test/install-config.yaml
> apiVersion: v1
> baseDomain: devcluster.openshift.com
> metadata:
> name: wking
> platform:
> aws:
> region: us-west-2
> pullSecret: |
> {
> REDACTED
> }
> EOF
$ AWS_PROFILE=openshift-dev ./openshift-install --dir testing create manifests
INFO Consuming "Install Config" from target directory
$ rm -f testing/openshift/99_openshift-cluster-api_master-machines-*.yaml
removed ‘testing/openshift/99_openshift-cluster-api_master-machines-0.yaml’
removed ‘testing/openshift/99_openshift-cluster-api_master-machines-1.yaml’
removed ‘testing/openshift/99_openshift-cluster-api_master-machines-2.yaml’
$ AWS_PROFILE=openshift-dev ./openshift-install --dir testing create ignition-configs
INFO Consuming "Openshift Manifests" from target directory
INFO Consuming "Common Manifests" from target directory
INFO Consuming "Master Machines" from target directory
INFO Consuming "Worker Machines" from target directory
$ echo "$?"
0So, where are we diverging?
There was a problem hiding this comment.
It's not clear to me why your version output includes release image registry.svc.ci.openshift.org/ocp/release:4.1.0-rc.0; oc adm release extract is supposed to be pinning that by digest, not by a potentially-floating tag.
What difference I only see is you was extracting installer from quay.io, but I was using some internal nightly pre-release build to extract installer.
I removed my previous installer bin, and extract it. This time, I can not reproduced either. Really interesting.
$ oc adm release extract --command='openshift-install' registry.svc.ci.openshift.org/ocp/release:4.1.0-rc.0
$ rm -f ./test1/openshift/99_openshift-cluster-api_master-machines-*.yaml
$ ./openshift-install create ignition-configs --dir test1
INFO Consuming "Master Machines" from target directory
INFO Consuming "Openshift Manifests" from target directory
INFO Consuming "Worker Machines" from target directory
INFO Consuming "Common Manifests" from target directory
There was a problem hiding this comment.
Per my testing result, both 'api' and 'api-int' are required.
There was a problem hiding this comment.
Ah, let me check my local shell script if update accordingly.
There was a problem hiding this comment.
Yeah, my fault, forget to change IgnitionLocation url accordingly. Ignore this comment.
There was a problem hiding this comment.
Hmm, seem using 'api-int' would hit x509 issue. Refer to https://bugzilla.redhat.com/show_bug.cgi?id=1697968#c13 more details.
79b05ec to
cf205b7
Compare
|
Based on some offline discussion, I've pushed 79b05ec63 -> cf205b774 removing the compute MachineSets as well. That gets us down to a single track (CloudFormation-launched compute nodes) in the docs and simplifies things (e.g. we no longer need the hairy |
By doing this, we no longer need the changes to 01_vpc.yaml. Can we revert those? Keeping the VPC to a minimal requirement (having kubernetes.io = shared tag on the subnets), we identify the needs of the ingress operator without being prescriptive in subnet naming and locking it to a cluster (the default manifests in the machine API was what was driving it). |
|
/retest |
|
/test e2e-openstack |
Folks are free to opt-in to the machine API during a UPI flow, but creating Machine(Set)s that match their host environment requires matching a few properties (subnet, securityGroups, ...). Our default templates are unlikely to do that out of the box, so just remove them with the standard flow. Users who want to wade in can do so, and I've adjusted our CloudFormation templates to set the same tags as our IPI assets to make this easier. But with the rm call, other folks don't have to worry about broken Machine(Set)s in their cluster confusing the machine API or other admins. The awkward join syntax for subnet names is because YAML doesn't support nesting !s [1]: You can't nest short form functions consecutively, so a pattern like !GetAZs !Ref is invalid. Also fix a few unrelated nits, e.g. the unused VpcId property in 06_cluster_worker_node.yaml. [1]: https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/intrinsic-function-reference-getavailabilityzones.html#w2ab1c21c24c36c17b8
Catching up with 13e4b70 (data/aws: create an api-int dns name, 2019-04-11, openshift#1601), now that 052fcee (asset/manifests: use internal apiserver name, 2019-04-17, openshift#1633) has moved some internal assets over to that name.
The point of these templates is to provide the bare minimum needed to get our cluster off the ground. Things like resource names and auxilliary tags are nice to have in a production deploy for admin orientation, but you can have a healthy cluster without them, so I'm culling them in this commit. Users, who may not be using our CloudFormation templates at all, should now have an easier time seeing what they need to set, and where they can go their own way. We need to keep the kubernetes.io/cluster/... tags on instances to avoid: May 01 21:31:53 ip-10-0-57-198 hyperkube[2311]: E0501 21:31:53.061462 2311 tags.go:95] Tag "KubernetesCluster" nor "kubernetes.io/cluster/..." not found; Kubernetes may behave unexpectedly.
To make it easier to recover these for: $ openshift-install gather bootstrap ...
|
/lgtm |
This isn't strictly required, because we're removing the resulting MachineSets right afterwards. It's setting the stage for a future where 'replicas: 0' means "no MachineSets" instead of "we'll make you some dummy MachineSets". And we can always remove the sed later if that future ends up not happening. The sed is based on [1], to replace 'replicas' only for the compute pool (and not the control-plane pool). While it should be POSIX-compliant (and not specific to GNU sed or other implementations), it is a bit finicky for a few reasons: * The range matching will not detect matches in the first line, but 'replicas' will always follow its parent 'compute', so we don't have to worry about first-line matches. * 'compute' sorts before 'controlPlane', so we don't have to worry about their 'replicas: ' coming first. * 'baseDomain' is the only other property that sorts before 'compute', but 'replicas: ' is not a legal substring for its domain-name value, so we don't have to worry about accidentally matching that. * While all of the above mean we're safe for now, this approach could break down if we add additional properties in the future that sort before 'compute' but do allow 'replicas: ' as a valid substring. [1]: https://stackoverflow.com/a/33416489
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: abhinavdahiya, wking 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 |
|
/retest |
We grew this in c22d042 (docs/user/aws/install_upi: Add 'sed' call to zero compute replicas, 2019-05-02, openshift#1649) to set the stage for changing the 'replicas: 0' semantics from "we'll make you some dummy MachineSets" to "we won't make you MachineSets". But that hasn't happened yet, and since 64f96df (scheduler: Use schedulable masters if no compute hosts defined, 2019-07-16, openshift#2004) 'replicas: 0' for compute has also meant "add the 'worker' role to control-plane nodes". That leads to racy problems when ingress comes through a load balancer, because Kubernetes load balancers exclude control-plane nodes from their target set [1,2] (although this may get relaxed soonish [3]). If the router pods get scheduled on the control plane machines due to the 'worker' role, they are not reachable from the load balancer and ingress routing breaks [4]. Seth says: > pod nodeSelectors are not like taints/tolerations. They only have > effect at scheduling time. They are not continually enforced. which means that attempting to address this issue as a day-2 operation would mean removing the 'worker' role from the control-plane nodes and then manually evicting the router pods to force rescheduling. So until we get the changes from [3], it's easier to just drop this section and keep the 'worker' role off the control-plane machines entirely. [1]: https://bugzilla.redhat.com/show_bug.cgi?id=1671136#c1 [2]: kubernetes/kubernetes#65618 [3]: https://bugzilla.redhat.com/show_bug.cgi?id=1744370#c6 [4]: https://bugzilla.redhat.com/show_bug.cgi?id=1755073
We grew replicas-zeroing in c22d042 (docs/user/aws/install_upi: Add 'sed' call to zero compute replicas, 2019-05-02, openshift#1649) to set the stage for changing the 'replicas: 0' semantics from "we'll make you some dummy MachineSets" to "we won't make you MachineSets". But that hasn't happened yet, and since 64f96df (scheduler: Use schedulable masters if no compute hosts defined, 2019-07-16, openshift#2004) 'replicas: 0' for compute has also meant "add the 'worker' role to control-plane nodes". That leads to racy problems when ingress comes through a load balancer, because Kubernetes load balancers exclude control-plane nodes from their target set [1,2] (although this may get relaxed soonish [3]). If the router pods get scheduled on the control plane machines due to the 'worker' role, they are not reachable from the load balancer and ingress routing breaks [4]. Seth says: > pod nodeSelectors are not like taints/tolerations. They only have > effect at scheduling time. They are not continually enforced. which means that attempting to address this issue as a day-2 operation would mean removing the 'worker' role from the control-plane nodes and then manually evicting the router pods to force rescheduling. So until we get the changes from [3], we can either drop the zeroing [5] or adjust the scheduler configuration to remove the effect of the zeroing. In both cases, this is a change we'll want to revert later once we bump Kubernetes to pick up a fix for the service load-balancer targets. [1]: https://bugzilla.redhat.com/show_bug.cgi?id=1671136#c1 [2]: kubernetes/kubernetes#65618 [3]: https://bugzilla.redhat.com/show_bug.cgi?id=1744370#c6 [4]: https://bugzilla.redhat.com/show_bug.cgi?id=1755073 [5]: openshift#2402
We grew replicas-zeroing in c22d042 (docs/user/aws/install_upi: Add 'sed' call to zero compute replicas, 2019-05-02, openshift#1649) to set the stage for changing the 'replicas: 0' semantics from "we'll make you some dummy MachineSets" to "we won't make you MachineSets". But that hasn't happened yet, and since 64f96df (scheduler: Use schedulable masters if no compute hosts defined, 2019-07-16, openshift#2004) 'replicas: 0' for compute has also meant "add the 'worker' role to control-plane nodes". That leads to racy problems when ingress comes through a load balancer, because Kubernetes load balancers exclude control-plane nodes from their target set [1,2] (although this may get relaxed soonish [3]). If the router pods get scheduled on the control plane machines due to the 'worker' role, they are not reachable from the load balancer and ingress routing breaks [4]. Seth says: > pod nodeSelectors are not like taints/tolerations. They only have > effect at scheduling time. They are not continually enforced. which means that attempting to address this issue as a day-2 operation would mean removing the 'worker' role from the control-plane nodes and then manually evicting the router pods to force rescheduling. So until we get the changes from [3], we can either drop the zeroing [5] or adjust the scheduler configuration to remove the effect of the zeroing. In both cases, this is a change we'll want to revert later once we bump Kubernetes to pick up a fix for the service load-balancer targets. [1]: https://bugzilla.redhat.com/show_bug.cgi?id=1671136#c1 [2]: kubernetes/kubernetes#65618 [3]: https://bugzilla.redhat.com/show_bug.cgi?id=1744370#c6 [4]: https://bugzilla.redhat.com/show_bug.cgi?id=1755073 [5]: openshift#2402
I'm still testing this, but folks keep talking about it, so pushing my current status somewhere public ;).