Skip to content

Conversation

@eranco74
Copy link
Contributor

No description provided.

@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: eranco74
To complete the pull request process, please assign deads2k after the PR has been reviewed.
You can assign the PR to them by writing /assign @deads2k in a comment when ready.

The full list of commands accepted by this bot can be found 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

@eranco74
Copy link
Contributor Author

/hold

@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 Dec 15, 2020
@eranco74 eranco74 force-pushed the bootstrap-in-place branch 2 times, most recently from 7a7e7b7 to 35c8438 Compare December 15, 2020 11:31
@eranco74 eranco74 mentioned this pull request Dec 15, 2020
@eranco74 eranco74 force-pushed the bootstrap-in-place branch 3 times, most recently from febe942 to 473909d Compare December 15, 2020 17:35
previousErrorSuffix := ""
var lastErr error
wait.Until(func() {
version, err := discovery.ServerVersion()
Copy link
Contributor

Choose a reason for hiding this comment

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

better check for /readyz, not discovey

Copy link
Contributor

@tsorya tsorya Dec 16, 2020

Choose a reason for hiding this comment

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

@sttts till api is not ready it will not answer the call, In case of kubeapi it is not really relevant what to check. We just need to have any call to return. We can though move it to http call but i think this method is nicely tested in installer and is good enough for our needs

Copy link
Contributor

Choose a reason for hiding this comment

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

It will answer before /readyz. But you might get ugly errors for certain calls.

Comment on lines 65 to 70
customTransport := http.DefaultTransport.(*http.Transport).Clone()
customTransport.TLSClientConfig = &tls.Config{InsecureSkipVerify: true}
client := &http.Client{Transport: customTransport}
previousErrorSuffix := ""
wait.Until(func() {
_, err := client.Get(fmt.Sprintf("https://%s/readyz", b.kubeApiUrl))

Choose a reason for hiding this comment

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

My preference was how you had it originally--looking up ServerVersion via the discovery client. I don't like using InsecureSkipVerify, despite it being pretty benign here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I prefer discovery client as well, this is less pretty but doesn't' require dependencies.
#46 (comment)

return err
}

return b.waitForApi()

Choose a reason for hiding this comment

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

Can you explain what problem this is looking to solve?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

in case required-pods="" (this is the case when running bootstrap-in-place) cluster-bootstrap will fail to publish an event since kube-apiserver isn't up yet.
This should solve the problem by ensuring the kube-apiserver is available when bootstrapControlPlane.Start() returns.
I'll add it to the commit message

@eranco74 eranco74 force-pushed the bootstrap-in-place branch 3 times, most recently from 82d09d9 to 80b33a6 Compare December 22, 2020 14:30
@eranco74
Copy link
Contributor Author

/unhold

tsorya and others added 3 commits December 27, 2020 17:39
in case required-pods="" (this is the case when running bootstrap-in-place) cluster-bootstrap may fail to publish an event in case kube-apiserver isn't up yet.
This should solve the problem by ensuring the kube-apiserver is available
This means that we fail to create some manifests
@eranco74 eranco74 force-pushed the bootstrap-in-place branch 6 times, most recently from 1ef73a3 to d3530db Compare December 29, 2020 07:05
eranco74 and others added 2 commits December 29, 2020 12:55
Use fcct for creating the master ignition

Signed-off-by: Eran Cohen <eran@stratoscale.com>
@tsorya tsorya force-pushed the bootstrap-in-place branch from cd5ba6f to e5d5e71 Compare January 5, 2021 17:28
@tsorya tsorya force-pushed the bootstrap-in-place branch from e5d5e71 to b3b043f Compare January 6, 2021 11:14
@eranco74
Copy link
Contributor Author

Closing, see the updated PR based on top of go.mod PR

@eranco74 eranco74 closed this Jan 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants