Bug 1866347: monitoring keepalive wrong format #89

YuviGold · 2020-08-17T17:26:29Z

Receiving mac address from the monitoring configuration file in yaml.
Deploying multiple assisted installers where the user does not provide the VIP addresses with the same cluster name ends up with them competing for the same DHCP leased address and conflicting.
The format should be as follows:

- name: api
  mac-address: ae:3e:38:f5:f8:15
  ip-address: 1.2.3.4
- name: ingress
  mac-address: 80:32:53:4f:cf:d6
  ip-address: 1.2.3.5

Watching the lease file with an inotify implementation https://github.com/fsnotify/fsnotify
In order to verify the dhclient made a renew for the lease instead of creating a new one.

…file instead of generating them

cybertron · 2020-08-17T17:41:59Z

Can you provide more details in the description on why these changes are being made?

pkg/monitor/lease.go

YuviGold · 2020-08-17T19:22:13Z

Can you provide more details in the description on why these changes are being made?

@cybertron Added. Full description can be found in Bugzilla

romfreiman · 2020-08-17T20:56:09Z

I think it worth mentioning in the PR the format of the expected file

pkg/monitor/lease.go

ori-amizur · 2020-08-18T07:03:51Z

pkg/monitor/lease.go

 	cmd := exec.Command("dhclient", "-d", "--no-pid", "-sf", "/bin/true",
-		"-lf", getLeaseFile(cfgPath, name), "-v", iface.Name, "-H", name)
+		"-lf", lease_file, "-v", iface.Name, "-H", name)


We are watching the file, but we will not know if the dhclient terminated for some reason. Shouldn't we monitor the process as well and maybe restart if it terminates?

pkg/monitor/lease.go

cybertron · 2020-08-18T16:13:54Z

/retitle Bug 1866347: monitoring keepalive wrong format

openshift-ci-robot · 2020-08-18T16:14:15Z

@YuviGold: This pull request references Bugzilla bug 1866347, which is invalid:

expected the bug to target the "4.6.0" release, but it targets "4.7.0" instead

Comment /bugzilla refresh to re-evaluate validity if changes to the Bugzilla bug are made, or edit the title of this pull request to link to a different bug.

In response to this:

Bug 1866347: monitoring keepalive wrong format

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

cybertron · 2020-08-18T16:26:07Z

Is this even a valid use case? Duplicate cluster names in a single DHCP domain are going to cause other issues besides this. Shouldn't we avoid that in the first place?

YuviGold · 2020-08-19T10:09:32Z

Is this even a valid use case? Duplicate cluster names in a single DHCP domain are going to cause other issues besides this. Shouldn't we avoid that in the first place?

@cybertron
In assisted-service - Users can create multiple clusters, and we do not know which of them will be connected to the same DHCP server, it will be difficult to make sure that we don't have duplicate names. Hence we use the clusterID.

bcrochet · 2020-08-19T20:53:11Z

Did all of the new vendoring really need to happen? Is that central to the fix?

cybertron · 2020-08-19T21:36:58Z

/retest

Failed on an infra issue.

cybertron

Okay, if I'm understanding this correctly then there are two primary changes here:
-Read a passed in MAC instead of generating one. I guess this pushes the responsibility for generating the MAC to someone else, so fair enough.
-Parse the lease file to determine whether there is a conflict with another cluster. I guess this is fine too. I'm still not convinced two clusters with the same name on the same network is going to work, but that's someone else's problem. :-)

I do see a few issues which I've left comments on inline. The log ones are mostly nitpicks that could be addressed in followups, but I'd like fixes or explanations for the others.

pkg/monitor/lease.go

cybertron · 2020-08-19T21:48:10Z

pkg/monitor/lease.go

 }

-func leaseVIPs(cfgPath string, clusterName string, vips []VIP) error {
+func LeaseVIPs(log logrus.FieldLogger, cfgPath string, clusterName string, vipMasterIface string, vips []vip, runInfinite bool) error {


Why are we passing a logger here? Can't this use the same one as the rest of the package?

It can use and it actually uses the same one when it is called from dynkeepalived.go

if err = LeaseVIPs(log, cfgPath, vipIface.Name, []vip{*vips.APIVip, *vips.IngressVip}, true); err != nil {

We want to use the leasing functions from the monitor package in the assisted-service as well.
You define your own log in the monitor package via var log = logrus.New().
In case another package with their own logger wants to use it (or even for it to be manipulated for the tests) it would be impossible without giving a log parameter.
Hence a log parameter is more flexible

In the future, when adding a new use case for code like this it would be a good thing to highlight in the PR description. That's important context for reviewers.

Personally, I would add new parameters to the end of the signature, not the beginning. Also, I'm not a huge fan of bare booleans. There is no context at the caller as to what a magic "true" or "false" might mean.

In the future, when adding a new use case for code like this it would be a good thing to highlight in the PR description. That's important context for reviewers.

@cybertron agreed

Personally, I would add new parameters to the end of the signature, not the beginning. Also, I'm not a huge fan of bare booleans. There is no context at the caller as to what a magic "true" or "false" might mean.

@bcrochet Seems fair. The boolean part has been removed anyway. But how would you implement that? Enums?

cybertron · 2020-08-19T22:00:48Z

pkg/monitor/lease.go

+	// --no-pid in order to allow running multiple dhclients
+	commandArgs := []string{"-v", iface.Name, "-H", name,
+		"-sf", "/bin/true", "-lf", leaseFile, "-d",
+		"--no-pid", "-pf", fmt.Sprintf("/var/run/dhclient.%s.pid", iface.Name)}


It doesn't make sense to pass both --no-pid and -pf. I'd be surprised if this does what you want.

--no-pid in order to allow running multiple dhclient simultaneously

-pf allow killing the process

Couldn't make it work without them both

pkg/monitor/lease.go

ori-amizur · 2020-08-19T23:32:07Z

pkg/monitor/lease.go

+		write := make(chan error)
+		defer close(write)
+
+		RunFiniteWatcher(log, watcher, leaseFile, iface.Name, ip, write)


I think that if you run with -1 flag, then it will run once, and then it will terminate. After that the lease file can be checked. Seems simpler flow. No need for goroutine, channel , and inotify usage.

It could be much simpler but unfortunately couldn't make it work

pkg/monitor/lease.go

cybertron · 2020-08-20T20:07:36Z

pkg/utils/fswatcher.go

+func CreateFileWatcher(log logrus.FieldLogger, fileName string) (*fsnotify.Watcher, error) {
+	watcher, err := fsnotify.NewWatcher()
+	if err != nil {
+		log.WithError(err).Error("Failed to add a create a new watcher")


Nit: I think "add a" should be removed from this log message.

cybertron · 2020-08-20T20:17:44Z

pkg/monitor/lease.go

 }

-func leaseVIPs(cfgPath string, clusterName string, vips []VIP) error {
+func LeaseVIPs(log logrus.FieldLogger, cfgPath string, clusterName string, vipMasterIface string, vips []vip, runInfinite bool) error {


In the future, when adding a new use case for code like this it would be a good thing to highlight in the PR description. That's important context for reviewers.

cybertron

/lgtm

One nit on a log message inline, but that can be fixed in a followup.

openshift-ci-robot · 2020-08-20T20:28:08Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cybertron, YuviGold

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~~ [cybertron]

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

cybertron · 2020-08-20T21:28:05Z

/bugzilla refresh

openshift-ci-robot · 2020-08-20T21:28:10Z

@cybertron: This pull request references Bugzilla bug 1866347, which is valid.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.6.0) matches configured target release for branch (4.6.0)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

In response to this:

/bugzilla refresh

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

openshift-ci-robot · 2020-08-20T21:31:55Z

@YuviGold: All pull requests linked via external trackers have merged: openshift/baremetal-runtimecfg#89. Bugzilla bug 1866347 has been moved to the MODIFIED state.

In response to this:

Bug 1866347: monitoring keepalive wrong format

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

YuviGold added 2 commits August 17, 2020 20:23

OCPBUGSM-13811 keepalived-monitoring gets its macs through a mounted …

6d46e86

…file instead of generating them

Watch lease file

96cba0c

openshift-ci-robot requested review from celebdor and yboaron August 17, 2020 17:26

romfreiman reviewed Aug 17, 2020

View reviewed changes

pkg/monitor/lease.go Show resolved Hide resolved

ori-amizur reviewed Aug 18, 2020

View reviewed changes

openshift-ci-robot changed the title ~~Bugfix/ocpbugsm 13811 monitoring keepalive wrong format~~ Bug 1866347: monitoring keepalive wrong format Aug 18, 2020

openshift-ci-robot added the bugzilla/severity-medium Referenced Bugzilla bug's severity is medium for the branch this PR is targeting. label Aug 18, 2020

openshift-ci-robot added the bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. label Aug 18, 2020

YuviGold force-pushed the bugfix/OCPBUGSM-13811-monitoring-keepalive-wrong-format branch from f89ddd4 to 870cd2e Compare August 19, 2020 10:06

YuviGold force-pushed the bugfix/OCPBUGSM-13811-monitoring-keepalive-wrong-format branch 3 times, most recently from 24c7c93 to 97aeb2b Compare August 19, 2020 12:19

cybertron requested changes Aug 19, 2020

View reviewed changes

ori-amizur reviewed Aug 19, 2020

View reviewed changes

pkg/monitor/lease.go Show resolved Hide resolved

YuviGold added 2 commits August 20, 2020 15:33

Support finite and infinite runs

02d5f4e

Update vendor

f036ccf

YuviGold force-pushed the bugfix/OCPBUGSM-13811-monitoring-keepalive-wrong-format branch from 97aeb2b to f036ccf Compare August 20, 2020 12:35

cybertron reviewed Aug 20, 2020

View reviewed changes

cybertron approved these changes Aug 20, 2020

View reviewed changes

openshift-ci-robot assigned cybertron Aug 20, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 20, 2020

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 20, 2020

openshift-ci-robot added bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. and removed bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. labels Aug 20, 2020

openshift-merge-robot merged commit b2b74d7 into openshift:master Aug 20, 2020

YuviGold deleted the bugfix/OCPBUGSM-13811-monitoring-keepalive-wrong-format branch August 21, 2020 09:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug 1866347: monitoring keepalive wrong format #89

Bug 1866347: monitoring keepalive wrong format #89

YuviGold commented Aug 17, 2020 •

edited

Loading

cybertron commented Aug 17, 2020

YuviGold commented Aug 17, 2020 •

edited

Loading

romfreiman commented Aug 17, 2020

ori-amizur Aug 18, 2020

cybertron commented Aug 18, 2020

openshift-ci-robot commented Aug 18, 2020

cybertron commented Aug 18, 2020

YuviGold commented Aug 19, 2020 •

edited

Loading

bcrochet commented Aug 19, 2020

cybertron commented Aug 19, 2020

cybertron left a comment

cybertron Aug 19, 2020

YuviGold Aug 20, 2020

cybertron Aug 20, 2020

bcrochet Aug 20, 2020

YuviGold Aug 21, 2020

cybertron Aug 19, 2020

YuviGold Aug 20, 2020

ori-amizur Aug 19, 2020 •

edited

Loading

YuviGold Aug 20, 2020

cybertron Aug 20, 2020

cybertron Aug 20, 2020

cybertron left a comment

openshift-ci-robot commented Aug 20, 2020

cybertron commented Aug 20, 2020

openshift-ci-robot commented Aug 20, 2020

openshift-ci-robot commented Aug 20, 2020

Bug 1866347: monitoring keepalive wrong format #89

Bug 1866347: monitoring keepalive wrong format #89

Conversation

YuviGold commented Aug 17, 2020 • edited Loading

cybertron commented Aug 17, 2020

YuviGold commented Aug 17, 2020 • edited Loading

romfreiman commented Aug 17, 2020

Choose a reason for hiding this comment

cybertron commented Aug 18, 2020

openshift-ci-robot commented Aug 18, 2020

cybertron commented Aug 18, 2020

YuviGold commented Aug 19, 2020 • edited Loading

bcrochet commented Aug 19, 2020

cybertron commented Aug 19, 2020

cybertron left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ori-amizur Aug 19, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cybertron left a comment

Choose a reason for hiding this comment

openshift-ci-robot commented Aug 20, 2020

cybertron commented Aug 20, 2020

openshift-ci-robot commented Aug 20, 2020

openshift-ci-robot commented Aug 20, 2020

YuviGold commented Aug 17, 2020 •

edited

Loading

YuviGold commented Aug 17, 2020 •

edited

Loading

YuviGold commented Aug 19, 2020 •

edited

Loading

ori-amizur Aug 19, 2020 •

edited

Loading