Bug: Wait for the certs to be mounted inside the container #2198

tenzen-y · 2023-08-05T23:36:10Z

What this PR does / why we need it:
To avoid the below error the cert-generator should wait for the certs to be mounted inside the container:

{"level":"error","ts":"2023-08-05T18:14:44Z","logger":"entrypoint","msg":"Unable to run the manager","error":"open /tmp/cert/tls.crt: no such file or directory","stacktrace":"main.main\n\t/go/src/github.com/kubeflow/katib/cmd/katib-controller/v1beta1/main.go:164\nruntime.main\n\t/usr/local/go/src/runtime/proc.go:250"}

Currently, the cert-generator immediately sends data to the certsReady channel once the certs are generated.
However, since completed to generate certs doesn't mean the mounted secret is updated with certs, the cert-generator should wait for the mounted secret to be updated by kubelet.

ref: https://kubernetes.io/docs/concepts/configuration/secret/#using-secrets-as-files-from-a-pod.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

Checklist:

Docs included if any changes are user facing

google-oss-prow · 2023-08-05T23:36:26Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: tenzen-y

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~~ [tenzen-y]

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

johnugeorge · 2023-08-13T11:21:16Z

pkg/certgenerator/v1beta1/generator.go

@@ -67,11 +70,34 @@ func (c *CertGenerator) Start(ctx context.Context) error {
 	if err := c.generate(ctx); err != nil {
 		return err
 	}
+	if err := wait.ExponentialBackoffWithContext(ctx, wait.Backoff{


Is there a requirement for a timeout? How does the user get to know that it is being waited for certs to be ready?

Is there a requirement for a timeout?

Yes, timeout is 5 minutes.

How does the user get to know that it is being waited for certs to be ready?

The controller pod doesn't ready until the cert is ready.

NAME READY STATUS RESTARTS AGE katib-controller-687798c75b-mtlz7 0/1 Running 0 4s status: conditions: - lastProbeTime: null lastTransitionTime: "2023-08-13T11:48:12Z" message: 'containers with unready status: [katib-controller]' reason: ContainersNotReady status: "False" type: ContainersReady controller@sha256:86697de94ac8dfbe53082beb96f1ea4ffea2bac982a2332719a99224e17335c7 lastState: {} name: katib-controller ready: false restartCount: 0 started: true state: running: startedAt: "2023-08-13T11:48:14Z"

I meant, how will the user know that Katib controller is waiting for certs to get ready?

The controller doesn't say, Waiting for certs to get ready. So, there are two ways that users can indirectly know whether the controller waits for certs to get ready:

As shown above, confirmation pod condition.

Creating the Experiments. If the controller waits for certs, users can get the following error:

Error from server (InternalError): error when creating "../../testdata/valid-experiment.yaml": Internal error occurred: failed calling webhook "defaulter.experiment.katib.kubeflow.org": could not get REST client: unable to load root certificates: unable to parse bytes as PEM block

Adding a log, Waiting for certs to get ready, to L72 in this file might be better.

Adding a log is better because user can understand the reason behind the issue

tenzen-y · 2023-08-14T16:10:08Z

pkg/certgenerator/v1beta1/generator.go

+	}
+	// We need to wait for certs to get ready even though using cert-manager.
+	klog.Info("Waiting for certs to get ready.")
+	if err := wait.ExponentialBackoffWithContext(ctx, wait.Backoff{


Even though we use cert-manager, the controller must wait for certs to be injected into the Secret resource.

andreyvelich · 2023-08-15T12:21:01Z

pkg/certgenerator/v1beta1/generator.go

+		namespace:           consts.DefaultKatibNamespace,
+		webhookServiceName:  config.WebhookServiceName,
+		webhookSecretName:   config.WebhookSecretName,
+		kubeClient:          mgr.GetClient(),
+		certsReady:          certsReady,
+		enableGenerateCerts: config.Enable,


Can we set fullServiceDomain here when we configure CertGenerator struct ?
Currently, we set it here, and I think we can specify the CertGenerator parameter in a single place.

Suggested change

namespace: consts.DefaultKatibNamespace,

webhookServiceName: config.WebhookServiceName,

webhookSecretName: config.WebhookSecretName,

kubeClient: mgr.GetClient(),

certsReady: certsReady,

enableGenerateCerts: config.Enable,

namespace: consts.DefaultKatibNamespace,

webhookServiceName: config.WebhookServiceName,

fullServiceDomain: strings.Join([]string{config.WebhookServiceName, consts.DefaultKatibNamespace, "svc"}, "."),

webhookSecretName: config.WebhookSecretName,

kubeClient: mgr.GetClient(),

certsReady: certsReady,

enableGenerateCerts: config.Enable,

It makes sense. Thanks!

andreyvelich · 2023-08-15T12:28:24Z

pkg/certgenerator/v1beta1/generator.go

+	}
+	// We need to wait for certs to get ready even though using cert-manager.
+	klog.Info("Waiting for certs to get ready.")
+	if err := wait.ExponentialBackoffWithContext(ctx, wait.Backoff{


@johnugeorge @tenzen-y Does it look confusing that we have this timeout in our generator package even if Katib Cert Generator doesn't used ?
Maybe we can add this timeout to the setupControllers section before we adding the Webhook to the manager ?

We can rename certsReady channel to certsGenerated to be more precise.

I think locating this timeout here would be better since this timeout is a function for the webhook certs.
However, I agree with your confusion. So renaming the certgenerator with certmanager or certcontroller might be better to clarify the purpose of this component.

@andreyvelich WDYT?

I think certcontroller would be better since certmanager conflicts context with cert-manager :-/

If we rename it to certcontroller how we can deal with Katib Config Settings ?
If user wants the enable cert generator they set the following setting:

certGenerator: enable: true

Ah, sorry. I will revert 69e9dfe since we can avoid the timeout error in the kubeflow installation by removing katib-webhook-cert Secret from kubeflow installation.

So, we wouldn't have to change the KatibConfig. WDYT?

So, the certcontroller will start only in the standalone installation (certGenerator.enable=true).

I think, if we are going to remove this timeout from Kubeflow installation, we need to name it as certgenerator since it is only internal logic that we have when Katib certGenerator is enabled.
Let's keep it as certgenerator for now and use this timeout only when our internal Cert Generator is used.
Does it sound good @tenzen-y ?

@andreyvelich That sounds good to me. Thanks for the great suggestion!

andreyvelich · 2023-08-15T12:34:27Z

pkg/certgenerator/v1beta1/generator.go

+	certFile := filepath.Join(consts.CertDir, serverCertName)
+	if _, err := os.Stat(certFile); err != nil {
+		return false, nil
+	}
+	keyFile := filepath.Join(consts.CertDir, serverKeyName)
+	if _, err := os.Stat(keyFile); err != nil {
+		return false, nil
+	}


Should we add the log line here as well ? E.g.

klog.Info("Public key file %v doesn't exist in the container yet", certFile) klog.Info("Private key file %v doesn't exist in the container yet", keyFile)

I hesitate to add the log since the logs will be output in large quantities and the controller's logs will be dirty.
So output logs every 30 sec or 15 sec might be better.
@andreyvelich WDYT?

Sure, that also works, if we can print them every 15 sec.

tenzen-y · 2023-08-15T14:06:11Z

@andreyvelich I addressed your suggestions. Please take another look.

andreyvelich · 2023-08-15T14:48:37Z

Thank you @tenzen-y!
/lgtm
/hold for review
/assign @johnugeorge

Signed-off-by: Yuki Iwai <[email protected]>

…ainer Signed-off-by: Yuki Iwai <[email protected]>

google-oss-prow · 2023-08-15T19:06:49Z

New changes are detected. LGTM label has been removed.

johnugeorge · 2023-08-15T19:19:28Z

/lgtm

…2198) * Wait for the certs to be mounted inside the container Signed-off-by: Yuki Iwai <[email protected]> * Initialize fullServiceDomain when adding certgenerator to the manager Signed-off-by: Yuki Iwai <[email protected]> * Output logs every 15 seconds if the certs don't yet exist in the container Signed-off-by: Yuki Iwai <[email protected]> --------- Signed-off-by: Yuki Iwai <[email protected]>

… inside the container (#2213) * Wait for the certs to be mounted inside the container * Initialize fullServiceDomain when adding certgenerator to the manager * Output logs every 15 seconds if the certs don't yet exist in the container --------- Signed-off-by: Yuki Iwai <[email protected]>

google-oss-prow bot added the size/M label Aug 5, 2023

google-oss-prow bot requested review from anencore94, gaocegege and johnugeorge August 5, 2023 23:36

google-oss-prow bot added the approved label Aug 5, 2023

tenzen-y force-pushed the wait-certs-mounted branch 2 times, most recently from 5e9e0f3 to cddd82d Compare August 6, 2023 16:48

johnugeorge reviewed Aug 13, 2023

View reviewed changes

tenzen-y force-pushed the wait-certs-mounted branch from cddd82d to 23dcbb5 Compare August 13, 2023 20:54

tenzen-y mentioned this pull request Aug 13, 2023

Add Label to Disable Katib Webhooks #2069

Closed

google-oss-prow bot added size/L and removed size/M labels Aug 14, 2023

tenzen-y commented Aug 14, 2023

View reviewed changes

andreyvelich reviewed Aug 15, 2023

View reviewed changes

tenzen-y force-pushed the wait-certs-mounted branch from 69e9dfe to fa81a60 Compare August 15, 2023 14:02

google-oss-prow bot assigned johnugeorge Aug 15, 2023

google-oss-prow bot added the do-not-merge/hold label Aug 15, 2023

google-oss-prow bot assigned andreyvelich Aug 15, 2023

google-oss-prow bot added the lgtm label Aug 15, 2023

tenzen-y added 3 commits August 16, 2023 03:17

Wait for the certs to be mounted inside the container

2d6cb2a

Signed-off-by: Yuki Iwai <[email protected]>

Initialize fullServiceDomain when adding certgenerator to the manager

3410d0f

Signed-off-by: Yuki Iwai <[email protected]>

Output logs every 15 seconds if the certs don't yet exist in the cont…

9d85d60

…ainer Signed-off-by: Yuki Iwai <[email protected]>

tenzen-y force-pushed the wait-certs-mounted branch from fa81a60 to 9d85d60 Compare August 15, 2023 18:17

johnugeorge removed the do-not-merge/hold label Aug 15, 2023

google-oss-prow bot removed the lgtm label Aug 15, 2023

andreyvelich added the lgtm label Aug 15, 2023

google-oss-prow bot merged commit 1b68744 into kubeflow:master Aug 15, 2023
58 checks passed

tenzen-y mentioned this pull request Aug 16, 2023

Automated cherry pick of #2198: Bug: Wait for the certs to be mounted inside the container #2213

Merged

1 task

tenzen-y deleted the wait-certs-mounted branch August 16, 2023 08:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug: Wait for the certs to be mounted inside the container #2198

Bug: Wait for the certs to be mounted inside the container #2198

tenzen-y commented Aug 5, 2023

google-oss-prow bot commented Aug 5, 2023

johnugeorge Aug 13, 2023

tenzen-y Aug 13, 2023

johnugeorge Aug 13, 2023

tenzen-y Aug 13, 2023 •

edited

Loading

johnugeorge Aug 13, 2023

tenzen-y Aug 13, 2023

tenzen-y Aug 14, 2023

andreyvelich Aug 15, 2023 •

edited

Loading

tenzen-y Aug 15, 2023

andreyvelich Aug 15, 2023

tenzen-y Aug 15, 2023

tenzen-y Aug 15, 2023

andreyvelich Aug 15, 2023

tenzen-y Aug 15, 2023 •

edited

Loading

tenzen-y Aug 15, 2023 •

edited

Loading

andreyvelich Aug 15, 2023

tenzen-y Aug 15, 2023

tenzen-y Aug 15, 2023

andreyvelich Aug 15, 2023

tenzen-y Aug 15, 2023

andreyvelich Aug 15, 2023

tenzen-y Aug 15, 2023

tenzen-y commented Aug 15, 2023

andreyvelich commented Aug 15, 2023

google-oss-prow bot commented Aug 15, 2023

johnugeorge commented Aug 15, 2023

Bug: Wait for the certs to be mounted inside the container #2198

Bug: Wait for the certs to be mounted inside the container #2198

Conversation

tenzen-y commented Aug 5, 2023

google-oss-prow bot commented Aug 5, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tenzen-y Aug 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andreyvelich Aug 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tenzen-y Aug 15, 2023 • edited Loading

Choose a reason for hiding this comment

tenzen-y Aug 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tenzen-y commented Aug 15, 2023

andreyvelich commented Aug 15, 2023

google-oss-prow bot commented Aug 15, 2023

johnugeorge commented Aug 15, 2023

tenzen-y Aug 13, 2023 •

edited

Loading

andreyvelich Aug 15, 2023 •

edited

Loading

tenzen-y Aug 15, 2023 •

edited

Loading

tenzen-y Aug 15, 2023 •

edited

Loading