Time synchronization inside LCOW UVM by ambarve · Pull Request #1119 · microsoft/hcsshim

ambarve · 2021-08-17T22:54:07Z

Currently LCOW UVM doesn't run any time synchronization service which causes a small amount of time lag after running the UVM for a long time (~10 days).

This change updates OpenGCS to start chronyd daemon as soon as gcs is started. It also adds a new annotation which can be used to disable this service inside the UVM.

ambarve · 2021-08-17T22:55:09Z

Do not merge this before we make the required changes to use the updated LCOW drop.

test/cri-containerd/runpodsandbox_test.go

internal/guest/commonutils/utilities.go

dcantah · 2021-08-18T20:01:48Z

internal/guest/commonutils/utilities.go

+	ptpDevPath := filepath.Join("/dev", filepath.Base(ptpDirPath))
+	chronydConfigString := fmt.Sprintf("refclock PHC %s poll 3 dpoll -2 offset 0\n", ptpDevPath)
+
+	chronydConfPath := "/tmp/chronyd.conf"


Might as well have this as a global const

That can hurt readability. If you're reading through code and see a global, you suddenly have to do extra work to figure out where it's being used. If it's only needed in this function then I would just keep it scoped to this function, though it could be moved to a const block in this function.

const in this function is fine with me. Global const in the communutils package might be a bit odd for this anyways, but truthfully maybe this entire function doesn't belong in communtils looking again

Oh you had the same feedback here haha #1119 (comment)

dcantah

lgtm!

internal/guest/commonutils/utilities.go

kevpar · 2021-08-18T20:07:38Z

internal/guest/commonutils/utilities.go

+		return errors.Wrap(err, "start chronyd command failed")
+	}
+	go func() {
+		waitErr := chronydCmd.Wait()


Do we need to handle cases like restarting this if it crashes or is OOM killed?

kevpar · 2021-08-18T20:08:32Z

Do not merge this before we make the required changes to use the updated LCOW drop.

In the future I think we can use a draft PR for cases like this.

kevpar · 2021-08-18T20:11:10Z

I wonder if there's any way we could run chronyd as a privileged infra container, and avoid needing to do this extra work in the gcs?

ambarve · 2021-10-26T04:48:35Z

@kevpar can you PTAL? (LCOW drop isn't ready yet but it would be good to get this ready to merge by the time we have it)

Changes to the opengcs to start the chronyd service after UVM boots. Signed-off-by: Amit Barve <ambarve@microsoft.com>

Signed-off-by: Amit Barve <ambarve@microsoft.com>

Add test to verify both chronyd running & disabled cases. Minor fixes in chronyd startup code. Signed-off-by: Amit Barve <ambarve@microsoft.com>

Signed-off-by: Amit Barve <ambarve@microsoft.com>

cmd/gcs/main.go

kevpar · 2021-11-10T21:59:18Z

cmd/gcs/main.go

+		}
+		// Expected clock name is `hyperv` so read first 6 chars and verify the
+		// name
+		buf := make([]byte, len(expectedClockName))


Is hyperv the entire contents of the clock_name file? If so may be easier to just use ioutil.ReadFile here.

Yes. The file we are looking for has only "hyperv" in it. But we loop over a list of directories and check the contents of a file named clock_name in each of those. We don't know what are the contents of other files. Doing ioutil.ReadFile on such files could take too much time and/or memory for no reason. That's why I followed this approach.

This seems okay, though if there is ever a new clock device added with clock_name of hyperv_newdevicedontuse then we could break. Maybe we should check the file size is what we expect too?

My suggestion to use ioutil.ReadFile was based on an assumption that clock names are probably going to be short. If we think that's safe to rely on then we could still take this approach.

I have changed this to use ioutil.ReadFile now. There is a small catch though, we now must also look for a new line at the end of the clock name. I have updated the expectedClockName accordingly.

cmd/gcs/main.go

Minor other fixes. Signed-off-by: Amit Barve <ambarve@microsoft.com>

Signed-off-by: Amit Barve <ambarve@microsoft.com>

cmd/gcs/main.go

Signed-off-by: Amit Barve <ambarve@microsoft.com>

kevpar

LGTM

cmd/gcs/main.go

Signed-off-by: Amit Barve <ambarve@microsoft.com>

dcantah

LGTM

Related work items: microsoft#1067, microsoft#1097, microsoft#1119, microsoft#1170, microsoft#1176, microsoft#1180, microsoft#1181, microsoft#1182, microsoft#1183, microsoft#1184, microsoft#1185, microsoft#1186, microsoft#1187, microsoft#1188, microsoft#1189, microsoft#1191, microsoft#1193, microsoft#1194, microsoft#1195, microsoft#1196, microsoft#1197, microsoft#1200, microsoft#1201, microsoft#1202, microsoft#1203, microsoft#1204, microsoft#1205, microsoft#1206, microsoft#1207, microsoft#1209, microsoft#1210, microsoft#1211, microsoft#1218, microsoft#1219, microsoft#1220, microsoft#1223

Start time synchronization service in opengcs Changes to the opengcs to start the chronyd service after UVM boots. Signed-off-by: Amit Barve <ambarve@microsoft.com> * Signed-off-by: Amit Barve <ambarve@microsoft.com> * TimeSync service inside LCOW UVM. Add test to verify both chronyd running & disabled cases. Minor fixes in chronyd startup code. Signed-off-by: Amit Barve <ambarve@microsoft.com> * Run Chronyd with restart monitor Signed-off-by: Amit Barve <ambarve@microsoft.com> * Force chronyd to step update time if difference is big Signed-off-by: Amit Barve <ambarve@microsoft.com> * Fixes after rebase Signed-off-by: Amit Barve <ambarve@microsoft.com> * go mod vendor & tidy Signed-off-by: Amit Barve <ambarve@microsoft.com> * Use backoff package instead of manually calculating backoffs Signed-off-by: Amit Barve <ambarve@microsoft.com> * Rename gcs cmdline params, use io.ReadFull instead of io.Read Minor other fixes. Signed-off-by: Amit Barve <ambarve@microsoft.com> * go mod vendor Signed-off-by: Amit Barve <ambarve@microsoft.com> * Ignore err if file doesn't exist Signed-off-by: Amit Barve <ambarve@microsoft.com> * Use ioutil.ReadFile to read clock_name file Signed-off-by: Amit Barve <ambarve@microsoft.com> * minor fix Signed-off-by: Amit Barve <ambarve@microsoft.com> * Remove incorrect usage of backoff.MaxElapsedTime Signed-off-by: Amit Barve <ambarve@microsoft.com>

ambarve requested a review from a team as a code owner August 17, 2021 22:54

dcantah reviewed Aug 18, 2021

View reviewed changes

test/cri-containerd/runpodsandbox_test.go Show resolved Hide resolved

dcantah reviewed Aug 18, 2021

View reviewed changes

internal/guest/commonutils/utilities.go Outdated Show resolved Hide resolved

dcantah reviewed Aug 18, 2021

View reviewed changes

internal/guest/commonutils/utilities.go Outdated Show resolved Hide resolved

dcantah reviewed Aug 18, 2021

View reviewed changes

dcantah approved these changes Aug 18, 2021

View reviewed changes

kevpar reviewed Aug 18, 2021

View reviewed changes

internal/guest/commonutils/utilities.go Outdated Show resolved Hide resolved

kevpar reviewed Aug 18, 2021

View reviewed changes

ambarve marked this pull request as draft August 25, 2021 18:04

ambarve force-pushed the chronyd_service branch from c69305b to 78f1615 Compare October 26, 2021 04:47

ambarve marked this pull request as ready for review October 26, 2021 04:49

ambarve added 6 commits November 8, 2021 10:41

Start time synchronization service in opengcs

9d21ad6

Changes to the opengcs to start the chronyd service after UVM boots. Signed-off-by: Amit Barve <ambarve@microsoft.com>

Add annotation to enable/disable Time synchronization service.

6decb3c

Signed-off-by: Amit Barve <ambarve@microsoft.com>

TimeSync service inside LCOW UVM.

7bb933e

Add test to verify both chronyd running & disabled cases. Minor fixes in chronyd startup code. Signed-off-by: Amit Barve <ambarve@microsoft.com>

Run Chronyd with restart monitor

51af604

Signed-off-by: Amit Barve <ambarve@microsoft.com>

Force chronyd to step update time if difference is big

ce87bc3

Signed-off-by: Amit Barve <ambarve@microsoft.com>

Fixes after rebase

08b0447

Signed-off-by: Amit Barve <ambarve@microsoft.com>

ambarve force-pushed the chronyd_service branch from 78f1615 to 08b0447 Compare November 8, 2021 20:38

go mod vendor & tidy

48fdb01

Signed-off-by: Amit Barve <ambarve@microsoft.com>

katiewasnothere assigned kevpar and dcantah Nov 8, 2021

Use backoff package instead of manually calculating backoffs

7e7eaf5

Signed-off-by: Amit Barve <ambarve@microsoft.com>

kevpar reviewed Nov 10, 2021

View reviewed changes

cmd/gcs/main.go Outdated Show resolved Hide resolved

kevpar requested changes Nov 10, 2021

View reviewed changes

ambarve added 2 commits November 10, 2021 16:09

Rename gcs cmdline params, use io.ReadFull instead of io.Read

f2ab351

Minor other fixes. Signed-off-by: Amit Barve <ambarve@microsoft.com>

go mod vendor

836127d

Signed-off-by: Amit Barve <ambarve@microsoft.com>

Ignore err if file doesn't exist

1015352

Signed-off-by: Amit Barve <ambarve@microsoft.com>

kevpar reviewed Nov 11, 2021

View reviewed changes

cmd/gcs/main.go Outdated Show resolved Hide resolved

ambarve added 2 commits November 10, 2021 21:15

Use ioutil.ReadFile to read clock_name file

d3013ce

Signed-off-by: Amit Barve <ambarve@microsoft.com>

minor fix

0d29698

Signed-off-by: Amit Barve <ambarve@microsoft.com>

kevpar approved these changes Nov 11, 2021

View reviewed changes

dcantah reviewed Nov 11, 2021

View reviewed changes

cmd/gcs/main.go Outdated Show resolved Hide resolved

Remove incorrect usage of backoff.MaxElapsedTime

2380361

Signed-off-by: Amit Barve <ambarve@microsoft.com>

ambarve force-pushed the chronyd_service branch from 241d4b7 to 2380361 Compare November 12, 2021 18:48

dcantah approved these changes Nov 12, 2021

View reviewed changes

ambarve merged commit db9908f into microsoft:master Nov 12, 2021

Conversation

ambarve commented Aug 17, 2021

Uh oh!

ambarve commented Aug 17, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dcantah left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kevpar Aug 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kevpar commented Aug 18, 2021

Uh oh!

kevpar commented Aug 18, 2021

Uh oh!

ambarve commented Oct 26, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kevpar left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dcantah left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

kevpar Aug 18, 2021 •

edited

Loading