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.

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

expose cluster load balancer spec and metadata via KubevirtCluster resource #80

Merged
merged 1 commit into from
Feb 2, 2022
Merged

expose cluster load balancer spec and metadata via KubevirtCluster resource #80

merged 1 commit into from
Feb 2, 2022

Conversation

agradouski
Copy link
Contributor

What this PR does / why we need it:
This PR enables the way to expose cluster load balancer spec via KubevirtCluster object. This allows users to specify different types of service for cluster load balancer, as opposed to it always being hardcoded to ClusterIP. cluster-template has an example of how to specify a service type, but it's optional. By default, ClusterIP service type is used.

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

Release notes:

NONE

@k8s-ci-robot k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jan 31, 2022
@coveralls
Copy link

coveralls commented Jan 31, 2022 •
edited
Loading

Pull Request Test Coverage Report for Build 1781479533

  • 6 of 29 (20.69%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.5%) to 41.106%
Changes Missing Coverage Covered Lines Changed/Added Lines %
api/v1alpha1/zz_generated.deepcopy.go 0 23 0.0%
Totals Coverage Status
Change from base Build 1779758705: -0.5%
Covered Lines: 587
Relevant Lines: 1428
💛 - Coveralls

@rmohr
Copy link
Contributor

rmohr commented Jan 31, 2022

/ok-to-test

@k8s-ci-robot k8s-ci-robot added the ok-to-test Indicates a non-member PR verified by an org member that is safe to test. label Jan 31, 2022
rmohr
rmohr suggested changes Jan 31, 2022
View reviewed changes
Copy link
Contributor

@rmohr rmohr left a comment

Choose a reason for hiding this comment

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

One recommendation.

api/v1alpha1/kubevirtcluster_types.go Outdated
// Service metadata.
ObjectMeta metav1.ObjectMeta `json:"metadata,omitempty"`
// Service specification.
Spec corev1.ServiceSpec `json:"spec,omitempty" valid:"required"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you think it would make sense to introduce here a ServiceSpecTemplate? Otherwise it is pretty hard to understand which fields are considered. It seems to only be the Type right now. Right? API validation would let pass all kinds of fields which are finally ignored.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes that makes sense. originally, I thought we could expose the entire service spec to users (and it theory that could also work, we could set selector and ports in the template, too), but at the same time we might not want to complicate KubevirtCluster spec too much, so, for now I just exposed the service type, and service metadata, with potential to expose other service spec parts in future if needed.

but agree, for now, we can just have our own ServiceSpecTemplate, to keep things explicit 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

also, note, that service spec is optional in KubevirtCluster object, and by default we fall back to the ClusterIP type as before. the main use case for now is being able to specify a different type (e.g. LoadBalancer) as well as setting labels and annotations for the service.

@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 1, 2022
@agradouski agradouski requested a review from rmohr February 1, 2022 07:14
@agradouski
Copy link
Contributor Author

@rmohr addressed your recommendation, please take another look

rmohr
rmohr reviewed Feb 1, 2022
View reviewed changes
Copy link
Contributor

@rmohr rmohr left a comment

Choose a reason for hiding this comment

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

One comment which may help users understanding what they exactly configure with this new spec. The implementation itself looks good to me!

api/v1alpha1/kubevirtcluster_types.go Outdated

// LoadBalancerServiceTemplate describes load balancer service template.
// +optional
LoadBalancerServiceTemplate LoadBalancerServiceTemplate `json:"loadBalancerServiceTemplate,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

It may make sense to explicitly state what exactly gets routed over this service. Like: apiserver traffic, ingress, ...

maybe it makes sense to not call this LodaBalancer... since in fact we can also choose non-load-balancer service types and instead use a name which hints what exactly is routed over this service.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

agree, will update. historically, we called it a "load balancer" in a sense of it being a load balancer for control plane nodes, even though as far as implementation goes it's a simple service. maybe make more sense to name it "control plane service" in the spec to be generic, and add more details in the api docs as you suggest 👍

@rmohr
Copy link
Contributor

rmohr commented Feb 1, 2022

here a retriggered e2e test which is green: https://prow.ci.kubevirt.io/view/gs/kubevirt-prow/pr-logs/pull/kubernetes-sigs_cluster-api-provider-kubevirt/80/pull-kubernetes-sigs-cluster-api-provider-kubevirt-e2e/1488464929697566720

We merged kubevirt/project-infra#1885 in kubevirts CI system which hopefully fix the GOPROXY issues from now on.

@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Feb 2, 2022
@agradouski agradouski requested a review from rmohr February 2, 2022 00:32
@agradouski
Copy link
Contributor Author

@rmohr addressed, please take another look

rmohr
rmohr approved these changes Feb 2, 2022
View reviewed changes
Copy link
Contributor

@rmohr rmohr left a comment

Choose a reason for hiding this comment

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

/lgtm

Looks great!

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 2, 2022
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: agradouski, rmohr

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

@k8s-ci-robot k8s-ci-robot merged commit 8d7bb3c into kubernetes-sigs:main Feb 2, 2022
@agradouski agradouski deleted the issue/64 branch February 2, 2022 17:08
nunnatsa pushed a commit to nunnatsa/cluster-api-provider-kubevirt that referenced this pull request Jan 12, 2025
@openshift-merge-bot
Merge pull request kubernetes-sigs#80 from openshift/appstudio-cluste…
fb12246 
…r-api-provider-kubevirt-mce-27

Red Hat Konflux update cluster-api-provider-kubevirt-mce-27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rmohr rmohr rmohr approved these changes

@neolit123 neolit123 Awaiting requested review from neolit123

Assignees

@rmohr rmohr

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Tenant cluster loadbalancer service is hardcoded to ClusterIP type
4 participants
@agradouski @coveralls @rmohr @k8s-ci-robot