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

IR-16: Implementing Leader Election #575

Merged
merged 2 commits into from Jul 8, 2020
Merged

IR-16: Implementing Leader Election #575

merged 2 commits into from Jul 8, 2020

Conversation

ricardomaraschini
Copy link
Contributor

Using controllercmd package to implement:

  1. Leader election
  2. Gain file watching capabilities.

We will migrate this operator to leverage this package.
@openshift-ci-robot openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 2, 2020
@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 2, 2020
@ricardomaraschini
Copy link
Contributor Author

/test e2e-aws-image-registry

cp: cannot create regular file '/var/run/secrets/ci.openshift.io/cluster-profile/day-2-manifests-insights-live.yaml': Read-only file system

@ricardomaraschini
Copy link
Contributor Author

/test e2e-aws

cp: cannot create regular file '/var/run/secrets/ci.openshift.io/cluster-profile/day-2-manifests-insights-live.yaml': Read-only file system

@ricardomaraschini
Copy link
Contributor Author

/test e2e-aws

1 similar comment
@ricardomaraschini
Copy link
Contributor Author

/test e2e-aws

@ricardomaraschini
Copy link
Contributor Author

/retest

@ricardomaraschini
Copy link
Contributor Author

/retest

Error: Unable to find matching route for Route Table (rtb-0beb26e67e23ea820) and destination CIDR block (0.0.0.0/0).

@ricardomaraschini
Copy link
Contributor Author

/retest

@ricardomaraschini ricardomaraschini changed the title WIP - IR-16: Implementing Leader Election IR-16: Implementing Leader Election Jul 7, 2020
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 7, 2020
@ricardomaraschini
Copy link
Contributor Author

/assign @dmage

Copy link
Member

@dmage dmage left a comment

Choose a reason for hiding this comment

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

few nits, otherwise looks good

cmd/cluster-image-registry-operator/main.go Outdated Show resolved Hide resolved
klog.Warningf("Unable to watch %s: %v", path, err)
continue
}
fileContents[path] = fcontent
Copy link
Member

Choose a reason for hiding this comment

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

You don't need to provide this map unless you want to avoid race conditions. But to avoid race conditions, the file consumer should provide the value that was read from the file.

If the consumer cannot provide it, these is no reason to read it manually.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was rather confusing but yeah, seems like if we don't pass it it calculates on its own. Patching it.

).WithLeaderElection(
configv1.LeaderElection{
LeaseDuration: metav1.Duration{
Duration: 15 * time.Second,
Copy link
Member

Choose a reason for hiding this comment

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

Can we use default values?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

seems like we can, patching it.

func(ctx context.Context, cctx *controllercmd.ControllerContext) error {
printVersion()
rand.Seed(time.Now().UnixNano())
go metrics.RunServer(metricsPort)
Copy link
Member

Choose a reason for hiding this comment

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

You can keep shopCh and pass ctx.Done()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That was my initial approach but I decided to get rid of it, it is not necessary.

Migrated to controllercmd so we can:

1. Have leader election
2. Have file watcher feature embed into the operator
@dmage
Copy link
Member

dmage commented Jul 8, 2020

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 8, 2020
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dmage, ricardomaraschini

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:
  • OWNERS [dmage,ricardomaraschini]

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

@openshift-merge-robot openshift-merge-robot merged commit e752804 into openshift:master Jul 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants