Add poll watcher support to spec watcher for M1 #8449

pozsgaic · 2022-02-17T19:06:19Z

Signed-off-by: pozsgaic pozsgai@progress.com

closes #8406

Note that this is only for SpecWatcher - there is more work for PeerWatcher and UserConfigWatcher

netlify · 2022-02-17T19:06:24Z

👷 Deploy Preview for chef-habitat processing.

🔨 Explore the source changes: 53a660f

🔍 Inspect the deploy log: https://app.netlify.com/sites/chef-habitat/deploys/62337a85d3c93f0009e7aead

atrniv

I've mentioned some changes that could ensure the code is more idiomatic

components/hab/src/command/studio/enter.rs

components/sup/src/manager/file_watcher.rs

components/sup/src/manager/sup_watcher.rs

themightychris · 2022-03-02T18:03:32Z

Doesn't the supervisor also have a file watcher for user.toml files? Is that covered here too?

Edit: looks like yes: https://github.com/habitat-sh/habitat/pull/8449/files#diff-dc64834de212928eec8772c9ea784a58977fd1fc5e5ffd10100dd930bc23ac0a

mwrock

I'm not sure these tests are getting run in CI. Looking at both of the linux sup builds, all the tests are filtered out according to the output. The windows tests are eunning the unit tests but these watcher tests are all linux only.

mwrock · 2022-03-09T19:26:05Z

components/sup/src/manager/peer_watcher.rs

+        }
+        assert_eq!(expected_members, members);
+        env::remove_var("HAB_STUDIO_HOST_ARCH");
+    }


let's extract the common code in the above 2 functions into a common function that both call

It seems that these tests need to be run in order otherwise the environmental variable will be incorrect.
By default rust will run cargo tests in multiple threads.
If I use the --test-threads=1 switch then the results are as expected.

UPDATE: Use locked_env_var! for concurrency for these tests.

It still looks like the above 2 tests are mostly the same. Was this overlooked?

No. The original was refactored by introducing the function peer_watcher_member_load_test(). These two tests use different values in for the Member constructors. We could pass in the raw data into the above function as well, but I did not think that it added value or readability.

mwrock · 2022-03-09T19:34:34Z

components/sup/src/manager/sup_watcher.rs

+        };
+        env::remove_var(TEST_STUDIO_HOST_ARCH_ENVVAR);
+        assert_eq!(watcher_type, "Fallback");
+    }


I think we also want a test validating that you get a native watcher when the env variable is not set.

These functions will need to be run sequentially to work. If we can run tests for this file in a single thread it should work.

UPDATE: Use locked_env_var! for concurrency for these tests.

mwrock · 2022-03-09T20:32:47Z

components/sup/src/manager/file_watcher.rs

+        }
+
+        env::remove_var("HAB_STUDIO_HOST_ARCH");
+    }


lets extract the common code from these 2 tests into a sharable function.

mwrock · 2022-03-09T21:00:59Z

components/sup/src/manager/file_watcher.rs

+            match &self.watcher_type {
+                WatcherType::NotifyWatcherType => false,
+                WatcherType::PollWatcherType => true,
+            }


do we need watcher_type? Could you use watcher.get_mut_underlying_watcher().real_watcher instead?

mwrock · 2022-03-09T21:23:32Z

components/sup/src/manager/file_watcher.rs

+                    1
+                } else {
+                    5
+                }


Here and all the other places where you are returning the number of events feels hacky to handle here because the interface is now misleasing. Seems like you are trying to return an arbitrary number of events for the pollwatcher which does not actually match up with any actual event count. Looks like this is all in service of bumping the iteration count later. Seems like things would be more clear and involve less code changes if you consolidated this in spin_watcher and have that multiply the iterations for the poll watcher.

This was refactored

Signed-off-by: pozsgaic <pozsgai@progress.com>

This reverts commit 0d2817a.

Signed-off-by: pozsgaic <pozsgai@progress.com>

mwrock · 2022-03-11T15:30:28Z

components/sup/src/manager/file_watcher.rs

    #[test]
    fn file_watcher() {
+        let lock = lock_env_var();
+        lock.set("");


this could just be lock.unset() which will clear out the variable.

OK - there are a number of other instances as well across our test cases, so will fix all of them.

Signed-off-by: pozsgaic <pozsgai@progress.com>

mwrock · 2022-03-14T21:37:31Z

components/sup/src/manager/file_watcher.rs

+        lock.set("aarch64-darwin");
+
+        //  When using the PollWatcher variant of SupWatcher, the
+        //  behavior is different than the NotifyWatcher.  The


can you briefly describe the difference and how that affects those cases?

Description updated in the code:
// When using the PollWatcher variant of SupWatcher, the
// behavior is different than the NotifyWatcher. The NotifyWatcher
// receives event callbacks in response to a file or directory change,
// while the PollWatcher must poll and check for changes.
// As such, there were observed differences in the timing and number
// of events that the PollWatcher will handle. It was determined
// through analyzing the output that the poll watcher as written is not handling
// test cases correctly after the second test case. It does not receive
// the required events to pass the test case regardless of timing.

mwrock · 2022-03-14T22:09:09Z

components/sup/src/manager/peer_watcher.rs

+        }
+        assert_eq!(expected_members, members);
+        env::remove_var("HAB_STUDIO_HOST_ARCH");
+    }


It still looks like the above 2 tests are mostly the same. Was this overlooked?

mwrock · 2022-03-14T23:10:34Z

components/sup/src/manager/file_watcher.rs

+            let mut iterations = expected_event_count;
+            if is_poll_watcher {
+                thread::sleep(Duration::from_secs(15));
+                iterations *= 5;


can you explain why the iteration count needs to be bumped?

"Through experimentation it was determined that the PollWatcher is less responsive
and emits more events than the NotifyWatcher. The initial sleep used in NotifyWatcher
was not adequate to pass the tests and was increased as a result. Also the number of iterations
required is larger for the PollWatcher case as there were intermediate events observed that would lead to
test case failure with the original iteration count used. The test cases will fail if the desired events are not emitted, so the iteration count was increased to account for the increased number of events in the PollWatcher."

we should be able to make this 3 iterations

mwrock · 2022-03-14T23:10:34Z

components/sup/src/manager/file_watcher.rs

+            let mut iterations = expected_event_count;
+            if is_poll_watcher {
+                thread::sleep(Duration::from_secs(15));
+                iterations *= 5;


can you explain why the iteration count needs to be bumped?

"Through experimentation it was determined that the PollWatcher is less responsive
and emits more events than the NotifyWatcher. The initial sleep used in NotifyWatcher
was not adequate to pass the tests and was increased as a result. Also the number of iterations
required is larger for the PollWatcher case as there were intermediate events observed that would lead to
test case failure with the original iteration count used. The test cases will fail if the desired events are not emitted, so the iteration count was increased to account for the increased number of events in the PollWatcher."

mwrock · 2022-03-14T23:11:08Z

components/sup/src/manager/file_watcher.rs

+
+            let mut iterations = expected_event_count;
+            if is_poll_watcher {
+                thread::sleep(Duration::from_secs(15));


15 seconds seems really high. Did we play much with smaller delays?

I originally got things working with a 30 second delay and then reduced it to 15 seconds and did not find it to be a problem. I can try. Will reduce if it will continue to pass the tests.

Update - 5 second sleep failed. 10 second sleep failed. Reverting to 15 second delay.

we should be able to bring this to 5

mwrock · 2022-03-14T23:11:08Z

components/sup/src/manager/file_watcher.rs

+
+            let mut iterations = expected_event_count;
+            if is_poll_watcher {
+                thread::sleep(Duration::from_secs(15));


15 seconds seems really high. Did we play much with smaller delays?

I originally got things working with a 30 second delay and then reduced it to 15 seconds and did not find it to be a problem. I can try. Will reduce if it will continue to pass the tests.

mwrock · 2022-03-14T23:20:47Z

components/sup/src/manager/file_watcher.rs

+        //  For the poll watcher tehre are more than one Debounced event received
+        //  and this test will fail.  Instead we can look at the last event and
+        //  ensure that it is correct, as it will be the last entry that will
+        //  determine the final state of the watched.


should we have both tests just look at the last one if that is the only one that matters?

Note the reason for testing only the last element in the PollWatcher tests as it can't test the entire set of events in order as the NotifyWatcher can. That would be a degradation of the NotifyWatcher test. It does work though if you think we should add it as a test for NotifyWatcher.

mwrock · 2022-03-14T23:20:47Z

components/sup/src/manager/file_watcher.rs

+        //  For the poll watcher tehre are more than one Debounced event received
+        //  and this test will fail.  Instead we can look at the last event and
+        //  ensure that it is correct, as it will be the last entry that will
+        //  determine the final state of the watched.


should we have both tests just look at the last one if that is the only one that matters?

Duplicate question

Signed-off-by: pozsgaic <pozsgai@progress.com>

mwrock · 2022-03-14T21:37:31Z

components/sup/src/manager/file_watcher.rs

+        lock.set("aarch64-darwin");
+
+        //  When using the PollWatcher variant of SupWatcher, the
+        //  behavior is different than the NotifyWatcher.  The


can you briefly describe the difference and how that affects the tests?

mwrock · 2022-03-17T17:33:32Z

components/sup/src/manager/file_watcher.rs

+
+                if is_poll_watcher {
+                    thread::sleep(Duration::from_secs(5));
+                };
                self.test_dirs(&step.dirs, &setup.watcher.paths.dirs);


based on my buildkite and local tests, we can remove this sleep.

mwrock · 2022-03-17T17:33:33Z

components/sup/src/manager/file_watcher.rs

+
+                if is_poll_watcher {
+                    thread::sleep(Duration::from_secs(5));
+                };
                self.test_dirs(&step.dirs, &setup.watcher.paths.dirs);


based on my buildkite and local tests, we can remove this sleep.

mwrock · 2022-03-17T17:34:27Z

components/sup/src/manager/file_watcher.rs

+
+            let mut iterations = expected_event_count;
+            if is_poll_watcher {
+                thread::sleep(Duration::from_secs(15));


we should be able to bring this to 5

mwrock · 2022-03-17T17:34:28Z

components/sup/src/manager/file_watcher.rs

+
+            let mut iterations = expected_event_count;
+            if is_poll_watcher {
+                thread::sleep(Duration::from_secs(15));


we should be able to bring this to 5

mwrock · 2022-03-17T17:35:12Z

components/sup/src/manager/file_watcher.rs

+            let mut iterations = expected_event_count;
+            if is_poll_watcher {
+                thread::sleep(Duration::from_secs(15));
+                iterations *= 5;


we should be able to make this 3 iterations

mwrock · 2022-03-17T17:35:12Z

components/sup/src/manager/file_watcher.rs

+            let mut iterations = expected_event_count;
+            if is_poll_watcher {
+                thread::sleep(Duration::from_secs(15));
+                iterations *= 5;


we should be able to make this 3 iterations

Signed-off-by: pozsgaic <pozsgai@progress.com>

chef-expeditor bot assigned pozsgaic Feb 17, 2022

pozsgaic requested review from atrniv, mwrock and sajjaphani February 17, 2022 19:07

pozsgaic force-pushed the cjp/spec_watcher_update branch 2 times, most recently from 63aca58 to d8ab682 Compare February 24, 2022 13:18

pozsgaic marked this pull request as ready for review February 24, 2022 13:19

atrniv reviewed Feb 24, 2022

View reviewed changes

atrniv approved these changes Mar 2, 2022

View reviewed changes

mwrock mentioned this pull request Mar 2, 2022

Supervisor crashes in Docker studio on M1 macbooks #8406

Closed

mwrock requested changes Mar 9, 2022

View reviewed changes

pozsgaic added 17 commits March 11, 2022 14:09

Refactor watchers and add pollwatcher support for Mac M1

c5f61ca

Signed-off-by: pozsgaic <pozsgai@progress.com>

code review enhancements

2c74e2d

Signed-off-by: pozsgaic <pozsgai@progress.com>

update to setting and using package target env variable

98e2fe0

Signed-off-by: pozsgaic <pozsgai@progress.com>

rustfmt

f98637f

Signed-off-by: pozsgaic <pozsgai@progress.com>

rustfmt

d7ca9d6

Signed-off-by: pozsgaic <pozsgai@progress.com>

rustfmt

2bf25f2

Signed-off-by: pozsgaic <pozsgai@progress.com>

clippy

4c94362

Signed-off-by: pozsgaic <pozsgai@progress.com>

clippy

74d08f2

Signed-off-by: pozsgaic <pozsgai@progress.com>

rustfmt

cb50a8b

Signed-off-by: pozsgaic <pozsgai@progress.com>

add test cases - file_watcher WIP

fefa482

Signed-off-by: pozsgaic <pozsgai@progress.com>

refactor file_watcher to support pollwatcher

d25e5d0

Signed-off-by: pozsgaic <pozsgai@progress.com>

rustfmt

9d969c5

Signed-off-by: pozsgaic <pozsgai@progress.com>

rustfmt

2645453

Signed-off-by: pozsgaic <pozsgai@progress.com>

rustfmt

8b0d6af

Signed-off-by: pozsgaic <pozsgai@progress.com>

clippy

0965fc4

Signed-off-by: pozsgaic <pozsgai@progress.com>

refactor to call drop on pollwatcher instances

83a255f

Signed-off-by: pozsgaic <pozsgai@progress.com>

fix clippy and remove drop code on test poll watcher

8f8d792

Signed-off-by: pozsgaic <pozsgai@progress.com>

pozsgaic added 7 commits March 11, 2022 14:09

rustfmt

92e95db

Signed-off-by: pozsgaic <pozsgai@progress.com>

test case updates

22cae13

Signed-off-by: pozsgaic <pozsgai@progress.com>

test case updates - formatting

ad66078

Signed-off-by: pozsgaic <pozsgai@progress.com>

test case updates - formatting

6e20335

Signed-off-by: pozsgaic <pozsgai@progress.com>

TEMP - add sup test no args

2d9ee6b

Signed-off-by: pozsgaic <pozsgai@progress.com>

Revert "TEMP - add sup test no args"

c701efc

This reverts commit 0d2817a.

update user_config_watcher

1802518

Signed-off-by: pozsgaic <pozsgai@progress.com>

pozsgaic force-pushed the cjp/spec_watcher_update branch from 9484f5c to 1802518 Compare March 11, 2022 14:43

mwrock reviewed Mar 11, 2022

View reviewed changes

update test code per review comments

9cec928

Signed-off-by: pozsgaic <pozsgai@progress.com>

mwrock requested changes Mar 14, 2022

View reviewed changes

pozsgaic added 2 commits March 15, 2022 18:11

improve descriptions for test cases

3498793

Signed-off-by: pozsgaic <pozsgai@progress.com>

rustfmt

78dcafa

Signed-off-by: pozsgaic <pozsgai@progress.com>

mwrock requested changes Mar 17, 2022

View reviewed changes

pozsgaic added 2 commits March 17, 2022 18:07

update delay times as recommended in test cases

b5762e4

Signed-off-by: pozsgaic <pozsgai@progress.com>

rustfmt

53a660f

Signed-off-by: pozsgaic <pozsgai@progress.com>

mwrock approved these changes Mar 17, 2022

View reviewed changes

pozsgaic merged commit 1ace224 into main Mar 18, 2022

Add poll watcher support to spec watcher for M1 #8449

Add poll watcher support to spec watcher for M1 #8449

Conversation

pozsgaic commented Feb 17, 2022 • edited by mwrock

netlify bot commented Feb 17, 2022 • edited

atrniv left a comment

Choose a reason for hiding this comment

themightychris commented Mar 2, 2022 • edited

mwrock left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pozsgaic Mar 10, 2022 • edited

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

pozsgaic Mar 10, 2022 • edited

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pozsgaic Mar 15, 2022 • edited

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

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

pozsgaic Mar 15, 2022 • edited

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

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

pozsgaic commented Feb 17, 2022 •

edited by mwrock

netlify bot commented Feb 17, 2022 •

edited

themightychris commented Mar 2, 2022 •

edited

pozsgaic Mar 10, 2022 •

edited

pozsgaic Mar 10, 2022 •

edited

pozsgaic Mar 15, 2022 •

edited

pozsgaic Mar 15, 2022 •

edited