Skip to content

Bug 1706689: asset/manifests: internal api server URL for infrastructure #1718

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
May 8, 2019
Merged

Bug 1706689: asset/manifests: internal api server URL for infrastructure #1718

merged 3 commits into from
May 8, 2019

Conversation

abhinavdahiya
Copy link
Contributor

Catch up to the api change introduced in openshift/api#308

@abhinavdahiya
vendor: bump openshift/api
6a61cd1 
Brins in changes from openshift/api#308

```console
$ dep version
dep:
 version     : v0.5.0
 build date  : 2018-07-26
 git hash    : 224a564
 go version  : go1.10.3
 go compiler : gc
 platform    : linux/amd64
 features    : ImportDuringSolve=false
$ dep ensure -update github.com/openshift/api github.com/openshift/client-go
```
@openshift-ci-robot openshift-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label May 7, 2019
@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 7, 2019
wking
wking reviewed May 7, 2019
View reviewed changes
cmd/openshift-install/create.go
@@ -326,6 +326,7 @@ func waitForInitializedCluster(ctx context.Context, config *rest.Config) error {
clusterVersionContext, cancel := context.WithTimeout(ctx, timeout)
defer cancel()

failing := configv1.ClusterStatusConditionType("Failing")
Copy link
Member

Choose a reason for hiding this comment

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

Is this just until the CVO moves to Degraded?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no plans for CVO to move to Degraded

Copy link
Member

Choose a reason for hiding this comment

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

We're just going to translate operator Degradeds into CVO Failings forever?

Copy link
Contributor Author

@abhinavdahiya abhinavdahiya May 7, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

no plans for CVO to move to Degraded
We're just going to translate operator Degradeds into CVO Failings forever?

forever i can't say ;)
for now yes

We're

the installer only cares about the fact that CVO reports failing, its only a consumer. the installer shouldn't care

Copy link
Member

Choose a reason for hiding this comment

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

every team was told to move. why didn't the cvo move?

@wking
Copy link
Member

wking commented May 7, 2019 •
edited
Loading

/lgtm

@deads2k thinks this might cause issues with in-cluster cert minting/rotating. But if it gets through CI it's fine with me, we just need to check/fix rotation before cutting a release.

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label May 7, 2019
abhinavdahiya added a commit to abhinavdahiya/machine-config-operator that referenced this pull request May 7, 2019
@abhinavdahiya
pkg: add migration for infrastructure api change
40858b1 
Catch up to the api change introduced in openshift/api#308
The migration is required until installer correctly sets up the infrastructure object in openshift/installer#1718
@abhinavdahiya abhinavdahiya mentioned this pull request May 7, 2019
Closed
@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label May 7, 2019
@abhinavdahiya abhinavdahiya mentioned this pull request May 7, 2019
Merged
@abhinavdahiya abhinavdahiya changed the title asset/manifests: set api and internal api server URL for infrastructure asset/manifests: internal api server URL for infrastructure May 7, 2019
@abhinavdahiya
asset/manifests: set internal api server URL for infrastructure
32ee8da 
Catch up to the api change introduced in [1]

PR [1] introduces breaking change for `.status.apiServerURL` therefore, this only adds the new
field `.status.apiServerInternalURL` so that consumers like KAS-O and MCO can move to this new field and
then installer can move the `.status.apiServerURL` to public url for Kubernetes API.

[1]: openshift/api#308
@abhinavdahiya
cmd: catch up to openshift/api dropping failing condition
7aa6482 
PR [1] dropped the Failing condition and CVO moved to still reports Failing by using a local definiton [2].

This defines a local definition for failing condition like in CVO.

[1]: openshift/api@a9fb3b1
[2]: openshift/cluster-version-operator@0b47fc9
@deads2k deads2k mentioned this pull request May 7, 2019
Merged
@wking
Copy link
Member

wking commented May 7, 2019

/lgtm

with the ratchet too ;)

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label May 7, 2019
@openshift-ci-robot
Copy link
Contributor

[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

Needs approval from an approver in each of these files:
  • OWNERS [abhinavdahiya,wking]

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

@abhinavdahiya
Copy link
Contributor Author

e2e-aws

failing console...

level=fatal msg="failed to initialize the cluster: Some cluster operators are still updating: authentication, console: timed out waiting for the condition"

/retest

@abhinavdahiya
Copy link
Contributor Author

/retest

@spadgett
Copy link
Member

spadgett commented May 7, 2019

e2e-aws

failing console...

level=fatal msg="failed to initialize the cluster: Some cluster operators are still updating: authentication, console: timed out waiting for the condition"

Looks like https://bugzilla.redhat.com/show_bug.cgi?id=1707510

@abhinavdahiya
Copy link
Contributor Author

/retest

@eparis eparis changed the title asset/manifests: internal api server URL for infrastructure Bug 1706689: asset/manifests: internal api server URL for infrastructure May 7, 2019
@wking
Copy link
Member

wking commented May 7, 2019

e2e-aws-upgrade:

fail [k8s.io/kubernetes/test/e2e/framework/service_util.go:589]: May  7 17:26:06.627: Timed out waiting for service "service-test" to have a load balancer
...
Failing tests:

[Disruptive] Cluster upgrade should maintain a functioning cluster [Feature:ClusterUpgrade] [Suite:openshift] [Serial]

I've reaped a few leaked resources, so these should go through now.

/retest

@deads2k deads2k mentioned this pull request May 7, 2019
Merged
@wking wking mentioned this pull request May 7, 2019
Closed
@eparis
Copy link
Member

eparis commented May 7, 2019

/retest

@wking
Copy link
Member

wking commented May 7, 2019 •
edited
Loading

/hold

We want this, but it's not a beta-blocker. Putting the brakes on to let #1720 pass us in the merge queue.

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 7, 2019
@eparis
Copy link
Member

eparis commented May 7, 2019

/retest

@sdodson
Copy link
Member

sdodson commented May 7, 2019

/hold cancel

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 7, 2019
abhinavdahiya added a commit to abhinavdahiya/installer that referenced this pull request May 8, 2019
@abhinavdahiya
asset/manifests: update infrastructure to set public apiserver url
5987f7e 
PR [1] kept the `apiServerURL` field unchanged ie set to internal url for Kubernetes API so that
the consumers of the field can move away from it. With KAS-O [2] change merged, we can move forward

[1]: openshift#1718
[2]: openshift/cluster-kube-apiserver-operator#465
@abhinavdahiya abhinavdahiya mentioned this pull request May 8, 2019
Merged
@abhinavdahiya
Copy link
Contributor Author

/retest

@openshift-merge-robot openshift-merge-robot merged commit 23aac52 into openshift:master May 8, 2019
@abhinavdahiya abhinavdahiya deleted the infra_api_changes branch May 8, 2019 16:45
@wking wking mentioned this pull request May 14, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wking wking wking left review comments

@eparis eparis eparis left review comments

@jcpowermac jcpowermac Awaiting requested review from jcpowermac

@patrickdillon patrickdillon Awaiting requested review from patrickdillon

Assignees

@wking wking

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/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@abhinavdahiya @wking @openshift-ci-robot @spadgett @eparis @sdodson @openshift-merge-robot