search

karmada-io / karmada

Open, Multi-Cloud, Multi-Cluster Kubernetes Orchestration
https://karmada.io
Apache License 2.0
4.12k stars 806 forks source link

enable mcs to support headless-svc #3372

Open wenchezhao opened 1 year ago

wenchezhao commented 1 year ago

What type of PR is this? /kind feature

What this PR does / why we need it: enable mcs to support headless-svc Which issue(s) this PR fixes: Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

chaunceyjiang commented 1 year ago

Just a question，why do you need to enable mcs to support headless-svc?

When ServiceImportType is Headless, it should be ignored

Refer to shouldIgnoreImport

codecov-commenter commented 1 year ago

Codecov Report

Merging #3372 (b1fbec5) into master (7589eae) will increase coverage by 0.00%. The diff coverage is n/a.

:mega: This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@           Coverage Diff           @@
##           master    #3372   +/-   ##
=======================================
  Coverage   51.61%   51.62%           
=======================================
  Files         210      210           
  Lines       18922    18922           
=======================================
+ Hits         9767     9768    +1     
+ Misses       8629     8628    -1     
  Partials      526      526
Flag Coverage Δ
unittests 51.62% <ø> (+<0.01%) :arrow_up:

Flags with carried forward coverage won't be shown. Click here to find out more.

see 1 file with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

wenchezhao commented 1 year ago

Just a question，why do you need to enable mcs to support headless-svc?

When ServiceImportType is Headless, it should be ignored

Refer to shouldIgnoreImport

I have a use case that requires mcs to support headless-svc, but I don’t understand why mcs-api doesn’t support it. However, I think this is a useful feature.

XiShanYongYe-Chang commented 1 year ago

I have a use case that requires mcs to support headless-svc, but I don’t understand why mcs-api doesn’t support it. However, I think this is a useful feature.

Thanks~ @wenchezhao

How do you use the headless service? Can you give your test report?

Maybe we can perfect our MCS documentation.

wenchezhao commented 1 year ago

How do you use the headless service? Can you give your test report?

Maybe we can perfect our MCS documentation.

Nothing special, a statefulset application, the svc is headless, and when the svc is exported, it also needs to be headless. Therefore, the type of ServiceImport needs to be Headless.

XiShanYongYe-Chang commented 1 year ago

Nothing special, a statefulset application, the svc is headless, and when the svc is exported, it also needs to be headless. Therefore, the type of ServiceImport needs to be Headless.

@wenchezhao, can you post your test report?

wenchezhao commented 1 year ago

@wenchezhao, can you post your test report?

hi, @XiShanYongYe-Chang this is my test yaml. If you need anything else, please let me know.

apiVersion: apps/v1
kind: StatefulSet
metadata:
  labels:
    app: web
  name: web
spec:
  replicas: 3
  selector:
    matchLabels:
      app: web
  serviceName: my-service
  template:
    metadata:
      labels:
        app: web
    spec:
      containers:
      - image: nginx
        imagePullPolicy: IfNotPresent
        name: web
        ports:
        - containerPort: 8000
---
apiVersion: v1
kind: Service
metadata:
  name: my-service
  annotations:
    mcs/localAnalysis: 'true'
spec:
  ports:
  - port: 80
    targetPort: 8000
  selector:
    app: web
  clusterIP: None
---
apiVersion: policy.karmada.io/v1alpha1
kind: PropagationPolicy
metadata:
  name: mynginx-workload
spec:
  resourceSelectors:
    - apiVersion: apps/v1
      kind: StatefulSet
      name: web
    - apiVersion: v1
      kind: Service
      name: my-service
  placement:
    clusterAffinity:
      clusterNames:
        - k8s86
        - k8s74
---
apiVersion: multicluster.x-k8s.io/v1alpha1
kind: ServiceExport
metadata:
  name: my-service
---
apiVersion: policy.karmada.io/v1alpha1
kind: PropagationPolicy
metadata:
  name: serve-export-policy
spec:
  resourceSelectors:
    - apiVersion: multicluster.x-k8s.io/v1alpha1
      kind: ServiceExport
      name: my-service
  placement:
    clusterAffinity:
      clusterNames:
        - k8s86
        - k8s74
---
apiVersion: multicluster.x-k8s.io/v1alpha1
kind: ServiceImport
metadata:
  name: my-service
spec:
#  type: ClusterSetIP
  type: Headless
  ports:
  - port: 80
    protocol: TCP
---
apiVersion: policy.karmada.io/v1alpha1
kind: PropagationPolicy
metadata:
  name: serve-import-policy
spec:
  resourceSelectors:
    - apiVersion: multicluster.x-k8s.io/v1alpha1
      kind: ServiceImport
      name: my-service
  placement:
    clusterAffinity:
      clusterNames:
        - k8s86
XiShanYongYe-Chang commented 1 year ago

Hi @wenchezhao, thanks for your quick response, have you successfully tested the headless service on your local site?

wenchezhao commented 1 year ago

Hi @wenchezhao, thanks for your quick response, have you successfully tested the headless service on your local site?

yes, after I made the changes, the headless-svc can be imported into the target cluster.

XiShanYongYe-Chang commented 1 year ago

Hi @wenchezhao, I'd like to know some more information, please don't mind.

How do you access the headless service in the import cluster, through the domain name? If there are two exported clusters, the endpoint obtained through domain name resolution should have two my-service-0, how do you solve this problem?

wenchezhao commented 1 year ago

How do you access the headless service in the import cluster, through the domain name? If there are two exported clusters, the endpoint obtained through domain name resolution should have two my-service-0, how do you solve this problem?

@XiShanYongYe-Chang yes, through the domain name. that’s exactly what I want to do next. I plan to write a coredns plugin to solve related problems.

XiShanYongYe-Chang commented 1 year ago

@XiShanYongYe-Chang yes, through the domain name. that’s exactly what I want to do next.

Thanks for you confirm.

I plan to write a coredns plugin to solve related problems.

Would you mind creating a new issue to track this task? And I'm interested in it, we can continue to discuss the solution.

wenchezhao commented 1 year ago

Would you mind creating a new issue to track this task? And I'm interested in it, we can continue to discuss the solution.

ok, no problem.

wenchezhao commented 1 year ago

squash the commits into one?

why squash the commits into one?I think these are two separate things, so I deliberately submitted them in two separate times.

XiShanYongYe-Chang commented 1 year ago

I understand why you set two commits. Currently, the new label does not seem to be used. Personally, a single commit may be confusing.

wenchezhao commented 1 year ago

I understand why you set two commits. Currently, the new label does not seem to be used. Personally, a single commit may be confusing.

I have already made the changes.

XiShanYongYe-Chang commented 1 year ago

I have already made the changes.

Thanks~

@RainbowMango Can you help rerun the e2e. I'm modifying the incorrect case.

RainbowMango commented 1 year ago

Done.

XiShanYongYe-Chang commented 1 year ago

/lgtm /approve /cc @RainbowMango

karmada-bot commented 1 year ago

[APPROVALNOTIFIER] This PR is NOT APPROVED

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

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files: - **[OWNERS](https://github.com/karmada-io/karmada/blob/master/OWNERS)** Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment
RainbowMango commented 1 year ago

@chaunceyjiang Do you have any comments?

chaunceyjiang commented 1 year ago

Just a suggestion, can we contribute this patch to upstream? See what upstream has to say about this patch.

RainbowMango commented 1 year ago

Just a suggestion, can we contribute this patch to upstream? See what upstream has to say about this patch.

Yes, we can talk about it with upstream guys. But, what's your opinion?

chaunceyjiang commented 1 year ago

My point is to let this pr wait for a while, the next release version of Karmada still has a long time. We can discuss clearly in the upstream before deciding whether to merge this pr.

chaunceyjiang commented 1 year ago

My core thought is that MCS should be consistent with upstream.