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

enable mcs to support headless-svc #3372

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

wenchezhao
Copy link

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


@karmada-bot karmada-bot added the kind/feature Categorizes issue or PR as related to a new feature. label Apr 7, 2023
@karmada-bot karmada-bot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Apr 7, 2023
@chaunceyjiang
Copy link
Member

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
Copy link

codecov-commenter commented Apr 7, 2023

Codecov Report

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

📣 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%) ⬆️

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
Copy link
Author

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
Copy link
Member

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
Copy link
Author

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.

Copy link
Member

@RainbowMango RainbowMango left a comment

Choose a reason for hiding this comment

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

@XiShanYongYe-Chang
Copy link
Member

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
Copy link
Author

wenchezhao commented Apr 10, 2023

@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
Copy link
Member

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

@wenchezhao
Copy link
Author

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
Copy link
Member

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
Copy link
Author

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
Copy link
Member

@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.

Copy link
Member

@XiShanYongYe-Chang XiShanYongYe-Chang left a comment

Choose a reason for hiding this comment

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

Generally LGTM, can you help squash the commits into one?

@wenchezhao
Copy link
Author

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
Copy link
Author

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
Copy link
Member

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

Signed-off-by: wenche <wenchuan.zhao@qq.com>
@wenchezhao
Copy link
Author

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
Copy link
Member

I have already made the changes.

Thanks~

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

@RainbowMango
Copy link
Member

Done.

@XiShanYongYe-Chang
Copy link
Member

/lgtm
/approve
/cc @RainbowMango

@karmada-bot karmada-bot added the lgtm Indicates that a PR is ready to be merged. label Apr 14, 2023
@karmada-bot
Copy link
Collaborator

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

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

Copy link
Member

@RainbowMango RainbowMango left a comment

Choose a reason for hiding this comment

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

/assign

@RainbowMango
Copy link
Member

@chaunceyjiang Do you have any comments?

@chaunceyjiang
Copy link
Member

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

@RainbowMango
Copy link
Member

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
Copy link
Member

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
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature Categorizes issue or PR as related to a new feature. lgtm Indicates that a PR is ready to be merged. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants