Expose periods of environment thread in waagent.conf #1891

narrieta · 2020-05-21T00:07:01Z

Added parameters in waagent.conf to control how often operations in the environment thread are performed.

codecov · 2020-05-21T00:09:16Z

Codecov Report

Merging #1891 into develop will decrease coverage by 0.05%.
The diff coverage is 58.26%.

@@             Coverage Diff             @@
##           develop    #1891      +/-   ##
===========================================
- Coverage    69.44%   69.38%   -0.06%     
===========================================
  Files           83       83              
  Lines        11445    11499      +54     
  Branches      1590     1598       +8     
===========================================
+ Hits          7948     7979      +31     
- Misses        3166     3183      +17     
- Partials       331      337       +6

Impacted Files	Coverage Δ
azurelinuxagent/ga/env.py	`50.86% <27.77%> (-0.54%)`	⬇️
azurelinuxagent/common/conf.py	`77.04% <57.69%> (-1.20%)`	⬇️
azurelinuxagent/ga/update.py	`87.71% <63.63%> (-0.95%)`	⬇️
azurelinuxagent/ga/monitor.py	`71.62% <80.00%> (-2.91%)`	⬇️
azurelinuxagent/common/event.py	`86.34% <100.00%> (+0.03%)`	⬆️
azurelinuxagent/ga/periodic_operation.py	`97.05% <100.00%> (+1.22%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d62c4c5...a989520. Read the comment docs.

README.md

larohra

Left some comments

larohra · 2020-05-21T16:32:16Z

azurelinuxagent/common/conf.py

@@ -164,6 +171,18 @@ def enable_firewall(conf=__conf__):
    return conf.get_switch("OS.EnableFirewall", False)


+def get_enable_firewall_period(conf=__conf__):
+    return conf.get_int("OS.EnableFirewallPeriod", 30)


A lot of the new flags dont have an entry in the README, any particular reason for that?

This should be the only one that is not in README. EnableFirewall is not there either so I am following the same

So there is a yml list and then there is a description for each flag in the conf file. I found a couple that have a description in the README but have not been added to the yml list in the README -

OS.RemovePersistentNetRulesPeriod OS.RootDeviceScsiTimeoutPeriod Provisioning.MonitorHostNamePeriod

Missing completely from README -

OS.MonitorDhcpClientRestartPeriod

Also I personally feel we should add the Firewall rules to the readme too, it makes more sense to be as transparent as possible especially because its an open source project.

I'll add MonitorDhcpClientRestartPeriod to README. The other list is not in sync with README, I think it is just meant as a sample, not a comprehensive list of options. I'm not planning on listing all the missing options there.

larohra · 2020-05-21T16:46:29Z

azurelinuxagent/ga/env.py

+                    for op in self._periodic_operations:
+                        op.run()
+                except Exception as e:
+                    logger.error("An error occurred in the environment thread main loop; will skip the current iteration.\n{0}", ustr(e))


We might never hit this path as no operation has their own error handling so the PeriodicOperation.run() would just log it and let it go.

I wonder if it would be cleaner to just keep only the uber except @line:119 to ensure that we log incase the thread goes down

a typo (for example) could produce an exception. since this is the main loop i am protecting it

larohra · 2020-05-21T16:48:11Z

azurelinuxagent/ga/monitor.py

+                        op.run()
+
+                except Exception as e:
+                    logger.error("An error occurred in the monitor thread main loop; will skip the current iteration.\n{0}", ustr(e))


Same for this, all functions have their own error handling or the PeriodicFunction.run will take care of it, this code path will never hit

same as above

BTW - the reason i added the try/except here is because sleep_until_next_operation in the finally was throwing on Python 2.6 (initially I was using timedelta.totalseconds() in it

Ahhh that makes sense, finally can throw too didnt think of that lol. Thanks for adding it! :)

larohra · 2020-05-21T16:54:10Z

README.md

+#### __Extensions.GoalStatePeriod__
+
+_Type: Integer_  
+_Default: 6_


Also just curious, why did you change the default from 3 to 6secs? I think Joe had a dependency on checking the artifact blob every 3 secs for Fasttrack.

We need to slow down status reporting. until it is moved to a different thread i'm slowing it down here. can you post a comment in joe's PR to not hardcode 3 sec

Sure, I'll add a comment once I do that PR

larohra · 2020-05-21T17:00:00Z

azurelinuxagent/ga/update.py

@@ -421,6 +422,28 @@ def _emit_restart_event(self):

        return

+    @staticmethod
+    def _emit_changes_in_default_configuration():


Thanks for adding this, much needed!
I have a suggestion, why not just log all parameters that changed from default, that way we will always have an idea of what the customers are using and maybe later on we can leverage that to change our defaults if needed.

larohra

LGTM

pgombar · 2020-05-21T21:32:50Z

azurelinuxagent/ga/periodic_operation.py

+        sleep_seconds = ((sleep_timedelta.days * 24 * 3600 + sleep_timedelta.seconds) * 10.0 ** 6 + sleep_timedelta.microseconds) / 10.0 ** 6
+
+        if sleep_seconds > 0:
+            time.sleep(sleep_seconds)


I had no idea sleep() could take floats...

pgombar · 2020-05-21T21:33:48Z

azurelinuxagent/ga/update.py

+            def log_if_int_changed_from_default(name, current):
+                default = conf.get_int_default_value(name)
+                if default != current:
+                    msg = "{0} changed from its default; new value: {1}".format(name, current)


Suggestion, log the default value here too?

i had it initially; too verbose, so i removed it

narrieta added 4 commits May 20, 2020 13:17

Expose periods of environment thread in waagent.conf

d3631af

Python 2.6 compatibility issues

fcfeb07

Report errors on the monitor/environment threads

cd3bfb2

Added new parameters to README

580d9e4

narrieta requested review from larohra, pgombar and ZhidongPeng as code owners May 21, 2020 00:07

larohra reviewed May 21, 2020

View reviewed changes

README.md Show resolved Hide resolved

larohra reviewed May 21, 2020

View reviewed changes

log changes in conf

9538b0c

larohra reviewed May 21, 2020

View reviewed changes

Document MonitorDhcpClientRestartPeriod

a3bfefa

larohra approved these changes May 21, 2020

View reviewed changes

pgombar reviewed May 21, 2020

View reviewed changes

pgombar approved these changes May 21, 2020

View reviewed changes

Merge branch 'develop' into env-thread

a989520

narrieta merged commit de81f1b into Azure:develop May 22, 2020

narrieta deleted the env-thread branch May 22, 2020 15:20

narrieta mentioned this pull request May 22, 2020

[BUG] Consistent CPU / disk usage from waagent run-exthandlers #1845

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expose periods of environment thread in waagent.conf #1891

Expose periods of environment thread in waagent.conf #1891

narrieta commented May 21, 2020

codecov bot commented May 21, 2020 •

edited

Loading

larohra left a comment

larohra May 21, 2020

narrieta May 21, 2020

larohra May 21, 2020

narrieta May 21, 2020

larohra May 21, 2020

narrieta May 21, 2020

larohra May 21, 2020

narrieta May 21, 2020

narrieta May 21, 2020

larohra May 21, 2020

larohra May 21, 2020

narrieta May 21, 2020

larohra May 21, 2020

larohra May 21, 2020

larohra left a comment

pgombar May 21, 2020

pgombar May 21, 2020

narrieta May 21, 2020

Expose periods of environment thread in waagent.conf #1891

Expose periods of environment thread in waagent.conf #1891

Conversation

narrieta commented May 21, 2020

codecov bot commented May 21, 2020 • edited Loading

Codecov Report

larohra 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

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

larohra 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

codecov bot commented May 21, 2020 •

edited

Loading