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

add sharding feature for ClusterSynchroManager #609

Merged
merged 1 commit into from
Dec 12, 2023

Conversation

jxustc
Copy link
Contributor

@jxustc jxustc commented Nov 28, 2023

What type of PR is this?
/kind feature

What this PR does / why we need it:
Implement the basic sharding feature of the ClusterSynchroManager

Which issue(s) this PR fixes:
Fixes #599

Special notes for your reviewer:
This is a PR for the V1.0 version of the ClusterSynchroManager sharding feature, based on the latest design version discussed previously
Does this PR introduce a user-facing change?:


@clusterpedia-bot
Copy link

Hi @jxustc,
Thanks for your pull request!
If the PR is ready, use the /auto-cc command to assign Reviewer to Review.
We will review it shortly.

Details

Instructions for interacting with me using comments are available here.
If you have questions or suggestions related to my behavior, please file an issue against the gh-ci-bot repository.

pkg/synchromanager/clustersynchro_manager.go Outdated Show resolved Hide resolved
pkg/synchromanager/clustersynchro_manager.go Outdated Show resolved Hide resolved
pkg/synchromanager/clustersynchro_manager.go Show resolved Hide resolved
pkg/synchromanager/clustersynchro_manager.go Outdated Show resolved Hide resolved
pkg/synchromanager/clustersynchro_manager.go Outdated Show resolved Hide resolved
pkg/synchromanager/clustersynchro_manager.go Outdated Show resolved Hide resolved
@Iceber
Copy link
Member

Iceber commented Dec 1, 2023

Rearranged the logic(pseudocode):

func reconcileCluster (){
   // To ensure coherent logic, we can add prejudgements in reconcileCluster
    if status.sharding == nil && spec.sharding != name{
        return
    }

    if status.sharding != nil && status.sharding != name {
        return
    }

    // The cluster will be in the following state:
    // spec.sharding == name and (status.sharding ==nil or status.sharding == name)
    // spec.sharding != name and status != nil and status.sharding == name


    if delele {
        do_delete....
        return
    }

    if spec.sharding != name {
        unmanage_and_set_nil()
        return
    }

    set_sharding_name()
    ...
}

It might be useful.

@jxustc
Copy link
Contributor Author

jxustc commented Dec 1, 2023

Rearranged the logic(pseudocode):

func reconcileCluster (){
   // To ensure coherent logic, we can add prejudgements in reconcileCluster
    if status.sharding == nil && spec.sharding != name{
        return
    }

    if status.sharding != nil && status.sharding != name {
        return
    }

    // The cluster will be in the following state:
    // spec.sharding == name and (status.sharding ==nil or status.sharding == name)
    // spec.sharding != name and status != nil and status.sharding == name


    if delele {
        do_delete....
        return
    }

    if spec.sharding != name {
        unmanage_and_set_nil()
        return
    }

    set_sharding_name()
    ...
}

It might be useful.

Thanks for the suggestion, I will improve it over the weekend

pkg/synchromanager/clustersynchro_manager.go Outdated Show resolved Hide resolved
pkg/synchromanager/clustersynchro_manager.go Show resolved Hide resolved
pkg/synchromanager/clustersynchro_manager.go Outdated Show resolved Hide resolved
pkg/synchromanager/clustersynchro_manager.go Outdated Show resolved Hide resolved
pkg/synchromanager/clustersynchro_manager.go Show resolved Hide resolved
Copy link
Member

@Iceber Iceber left a comment

Choose a reason for hiding this comment

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

Others look good, only a little bit of modification needed

pkg/synchromanager/clustersynchro_manager.go Outdated Show resolved Hide resolved
pkg/synchromanager/clustersynchro_manager.go Outdated Show resolved Hide resolved

if cluster.Status.ShardingName != nil && *cluster.Status.ShardingName != manager.shardingName {
return controller.NoRequeueResult
}
Copy link
Member

Choose a reason for hiding this comment

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

I think this could be put into a separate function, which would add more comprehensive unit testing to it.
But it can be optimized in a new pr.

pkg/synchromanager/clustersynchro_manager.go Outdated Show resolved Hide resolved
cmd/clustersynchro-manager/app/options/options.go Outdated Show resolved Hide resolved
Signed-off-by: jixiang <2623210647@qq.com>

make codegen

Signed-off-by: jixiang <2623210647@qq.com>

make crds

Signed-off-by: jixiang <2623210647@qq.com>

add zz_generated.deepcopy.go

Signed-off-by: jixiang <2623210647@qq.com>

resolve some conversations and rearrange the code

Signed-off-by: jixiang <2623210647@qq.com>

format code

Signed-off-by: jixiang <2623210647@qq.com>

resolve some conversations

Signed-off-by: jixiang <2623210647@qq.com>

resolve some conversations

Signed-off-by: jixiang <2623210647@qq.com>
@@ -160,13 +162,18 @@ func (o *Options) Config() (*config.Config, error) {
}
kubeStateMetricsServerConfig := o.KubeStateMetrics.ServerConfig(metricsConfig)

if o.ShardingName != "" {
o.LeaderElection.ResourceName = fmt.Sprintf("%s-%s", o.LeaderElection.ResourceName, o.ShardingName)
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to check whether the resource already exists here?

Copy link
Member

Choose a reason for hiding this comment

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

The leaderelection will take care of checking for the resource, and if it already exists then it will race for it.

@Iceber Iceber changed the title sharding feature v1.0 for ClusterSynchroManager add sharding feature for ClusterSynchroManager Dec 5, 2023
Copy link
Member

@Iceber Iceber left a comment

Choose a reason for hiding this comment

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

LGTM, Thanks

@Iceber Iceber merged commit 810c6c4 into clusterpedia-io:main Dec 12, 2023
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

sharding feature for ClusterSynchroManager
4 participants