Skip to content
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

Consolidate the katib-cert-generator to the katib-controller #2185

Merged
merged 14 commits into from
Aug 4, 2023

Conversation

tenzen-y
Copy link
Member

@tenzen-y tenzen-y commented Jul 31, 2023

What this PR does / why we need it:
I modified the katib-controller and removed the katib-cert-generator to consolidate the katib-cert-generator to the katib-controller.

By this PR, katib is started in the following steps:

  1. All probe servers are registered to the manager.
  2. Start the manager.
  3. Start cert-generator as a part of the manager.
  4. Controllers and webhooks are registered to the manager after the certs are generated (certsReady channel is closed)

In the katib-controller, I mainly worked on the below tasks:

  1. Expand the KatibConfig API like this:
---
apiVersion: config.kubeflow.org/v1beta1
kind: KatibConfig
init:
+ certGenerator:
+   enable: true
+   webhookServiceName: test-svc
+   webhookSecretName: test-secret
  controller:
    webhookPort: 8443
...
  1. Add the cert-generator to the manager as a Runnable:

// CertGenerator is the manager to generate certs.
type CertGenerator struct {
namespace string
serviceName string
secretName string
kubeClient client.Client
certsReady chan struct{}
certs *certificates
fullServiceDomain string
}
var _ manager.Runnable = &CertGenerator{}
var _ manager.LeaderElectionRunnable = &CertGenerator{}
func (c *CertGenerator) Start(ctx context.Context) error {
if err := c.generate(ctx); err != nil {
return err
}
// Close a certsReady means start to register controllers to the manager.
close(c.certsReady)
return nil
}
func (c *CertGenerator) NeedLeaderElection() bool {
return true
}
// AddToManager adds the cert-generator to the manager.
func AddToManager(mgr manager.Manager, config configv1beta1.CertGeneratorConfig, certsReady chan struct{}) error {
return mgr.Add(&CertGenerator{
namespace: consts.DefaultKatibNamespace,
serviceName: config.WebhookServiceName,
secretName: config.WebhookSecretName,
kubeClient: mgr.GetClient(),
certsReady: certsReady,
})
}

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 #2149

/hold
Blocked on #2176

Checklist:

  • Docs included if any changes are user facing

@google-oss-prow
Copy link

[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:

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

@tenzen-y tenzen-y force-pushed the consolidate-cert-generator branch 3 times, most recently from 3b1d5a3 to 3dbf928 Compare August 1, 2023 02:21
@tenzen-y tenzen-y changed the title WIP: Consolidate the katib-cert-generator to the katib-controller Consolidate the katib-cert-generator to the katib-controller Aug 1, 2023
@tenzen-y tenzen-y force-pushed the consolidate-cert-generator branch 2 times, most recently from 5040d0a to 622ec24 Compare August 1, 2023 11:48
Comment on lines 139 to 149
if err := mgr.AddHealthzCheck("healthz", healthz.Ping); err != nil {
log.Error(err, "Unable to add healthz endpoint to the manager")
os.Exit(1)
}
Copy link
Member Author

@tenzen-y tenzen-y Aug 1, 2023

Choose a reason for hiding this comment

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

I removed this health check since this is duplicated with L154-L157.

	if err = mgr.AddHealthzCheck("healthz", hookServer.StartedChecker()); err != nil {
		log.Error(err, "Add webhook server health checker to the manager failed")
		os.Exit(1)
	}

Copy link
Member

@andreyvelich andreyvelich left a comment

Choose a reason for hiding this comment

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

Thank you for this great contribution @tenzen-y!
Please can you rebase your PR so I can review it this week.

@tenzen-y
Copy link
Member Author

tenzen-y commented Aug 1, 2023

Thank you for this great contribution @tenzen-y!
Please can you rebase your PR so I can review it this week.

Sure.

@tenzen-y
Copy link
Member Author

tenzen-y commented Aug 1, 2023

I will rebase this PR once #2176 is merged into the master branch.

@tenzen-y tenzen-y force-pushed the consolidate-cert-generator branch 7 times, most recently from 83ec74e to ed91adb Compare August 1, 2023 19:59
@tenzen-y
Copy link
Member Author

tenzen-y commented Aug 1, 2023

@andreyvelich This PR is ready for review. Please take a look.

Copy link
Member

@andreyvelich andreyvelich left a comment

Choose a reason for hiding this comment

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

Thank you for this great improvement @tenzen-y!
I left a few initial comments.

cmd/katib-controller/v1beta1/main.go Outdated Show resolved Hide resolved
cmd/katib-controller/v1beta1/main.go Show resolved Hide resolved
cmd/katib-controller/v1beta1/main.go Show resolved Hide resolved
cmd/katib-controller/v1beta1/main.go Show resolved Hide resolved
manifests/v1beta1/components/controller/controller.yaml Outdated Show resolved Hide resolved
pkg/cert-generator/v1beta1/generator.go Outdated Show resolved Hide resolved
pkg/apis/config/v1beta1/types.go Outdated Show resolved Hide resolved
pkg/apis/config/v1beta1/types.go Outdated Show resolved Hide resolved
pkg/cert-generator/v1beta1/generator.go Outdated Show resolved Hide resolved
pkg/controller.v1beta1/consts/const.go Outdated Show resolved Hide resolved
@tenzen-y
Copy link
Member Author

tenzen-y commented Aug 4, 2023

Rebased.

@johnugeorge
Copy link
Member

/lgtm

@johnugeorge
Copy link
Member

/hold cancel

@google-oss-prow google-oss-prow bot merged commit 06740a0 into kubeflow:master Aug 4, 2023
58 checks passed
@andreyvelich
Copy link
Member

Thank you for this great improvement @tenzen-y 🎉 .
Let me rebase my PR: #2018

@tenzen-y tenzen-y deleted the consolidate-cert-generator branch August 4, 2023 19:45
@tenzen-y
Copy link
Member Author

tenzen-y commented Aug 4, 2023

Thanks, everyone!

orfeas-k added a commit to canonical/katib-rocks that referenced this pull request Sep 22, 2023
Remove this ROCK since upstream has consolidated it into the
katib-controller.
Ref kubeflow/katib/pull/2185
orfeas-k added a commit to canonical/katib-rocks that referenced this pull request Sep 29, 2023
… 1.8 (#33)

Bumped all ROCKs with following exceptions:
- `cert-generator` ROCK is completely removed since corresponding component 
was consolidated into `katib-controller` component. See kubeflow/katib#2185
- `katib-ui-rock` needs to be rewritten since it is now based on wrong Dockerfile. Thus,
no changes were introduced in this ROCK. See #32
- `katib-db-manager`: Diverged from upstream by keeping binary for GRPC 
health checks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consolidate katib-cert-generator to katib-controller
3 participants