Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Add SpannerAutoscaleSchedule API #71

Merged
merged 6 commits into from
Feb 15, 2022
Merged

Conversation

rustycl0ck
Copy link
Contributor

The main purpose of this PR is to introduce all the boilerplate code, so that reviewing smaller functionality improvements and feature implementations in future PRs will be easier.

Contents of this PR:

  • Add the SpannerAutoscaleSchedule API boilerplate code
  • Add SpannerAutoscaleSchedule API schema/structure
  • Convert the existing functional patterns to support multiple types.
    • Before: WithLog can not be used for initializing SpannerAutoscaleScheduleReconciler because it conflicts with the already present WithLog functional option for SpannerAutoscalerReconciler.
    • After: WithLog (or any other options) can now be easily reused for multiple types
  • Implement basic functionality of adding a SpannerAutoscaleSchedule to the SpannerAutoscaler.Status.Schedules list (just as a way of sanity check)

@rustycl0ck rustycl0ck requested a review from a team as a code owner February 4, 2022 05:30
kind: CustomResourceDefinition
metadata:
annotations:
cert-manager.io/inject-ca-from: $(CERTIFICATE_NAMESPACE)/$(CERTIFICATE_NAME)
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like you need to add vars to config/crd/kustomization.yaml like config/default/kustomization.yaml.

Copy link
Contributor Author

@rustycl0ck rustycl0ck Feb 8, 2022

Choose a reason for hiding this comment

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

Yes, make deploy (installs the CRD + all other resources) works, but make install does not work because of the missing variables currently. I think this is a scaffolding issue in the Kubebuilder.
As a work-around, I have been using make undeploy deploy (re-installs the CRD as well as all the other resources) or just re-create the Kind cluster sometimes.

To fix this, I will try to find if it is a scaffolding issue or just a documentation issue in Kubebuilder.

For our case, the webhook manifests for SpannerAutoscaleSchedule have not been generated yet. When I enable the webhooks for SpannerAutoscaleSchedule, then I will add these variables too in our configs.

Signed-off-by: rustyclock <rustyclock@protonmail.com>
Signed-off-by: rustyclock <rustyclock@protonmail.com>
Signed-off-by: rustyclock <rustyclock@protonmail.com>
Signed-off-by: rustyclock <rustyclock@protonmail.com>
@rustycl0ck
Copy link
Contributor Author

Rebased with master to keep the commit history clean. Please have a look again whenever you have time 🙇

Comment on lines 104 to 105
log.Info("registering schedule with spanner-autoscaler", "schedule", sas.Name, "autoscaler", sa.Name)
log.V(1).Info("registering schedule with spanner-autoscaler", "schedule", sas, "autoscaler", sa)
Copy link
Contributor

Choose a reason for hiding this comment

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

One of these logs seems to be good enough.

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 kept two because Debug level logs the full objects, which can be useful for finding issues during local development etc. And at the Info level in production, just the names of the objects should be sufficient to confirm that the newly created schedule was successfully registered with the corresponding autoscaler. No extra clutter there.

I have now removed the Debug level log.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see, thank you for your comment!

tkuchiki and others added 2 commits February 15, 2022 17:36
Signed-off-by: rustyclock <rustyclock@protonmail.com>
@rustycl0ck rustycl0ck merged commit f95d319 into mercari:master Feb 15, 2022
@rustycl0ck rustycl0ck added the enhancement New feature or request label Mar 14, 2022
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
enhancement New feature or request size/XL
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants