Cont rkt tip aug 1st #168

spatchineelam · 2019-08-01T15:33:06Z

Squashed commits into two commits based on the owner.
Softlink /persist/rkt to /var/lib/rkt
Use Add rkt data directory global option --dir=<> for all rkt commands
And rebased the patches on Top of master

zedvijay · 2019-08-02T15:10:00Z

pkg/pillar/cmd/zedmanager/updatestatus.go

+				log.Debugf("doInstall: empty Dpath")
+			}
+			if len(strings.TrimSpace(ss.Name)) > 0 {
+				downloadUrl += "/" + ss.Name


golang path library does this for you.

Ah.. ok.. Will use that.

zedvijay · 2019-08-02T15:10:21Z

pkg/pillar/cmd/zedmanager/updatestatus.go

+			}
+			log.Infof("doInstall: downloadUrl: %s", downloadUrl)
+
+			if ss.Format == "8" {


8 is format container. This really should be enum instead of a string. Was thinking of making that change separately.

zedvijay · 2019-08-02T15:11:24Z

pkg/pillar/cmd/zedmanager/handledomainmgr.go

-		if err != nil {
-			return err
+		location := ""
+		err := errors.New("")


I didn't see this style of err, initliazation. Is empty string nil error ?

It is not actually being used. I just need it declared. I think the right way is to declare it as
var err errors.Error
err = nil

zedvijay · 2019-08-02T15:14:15Z

pkg/pillar/types/downloadertypes.go

 )

+type RktCredentials struct {


This is interesting, why does downloader know its container credentials, why cannot we use common user/name password location, does there different sturcture for SFTP ?

For other types, we are directly passing it to the command. For rkt, we need to put that into a json file and pass the file. Hence doing it this way.

zedvijay · 2019-08-02T15:16:49Z

pkg/pillar/cmd/downloader/downloader.go

+}
+
+func rktCreateAuthFile(config *types.DownloaderConfig) (string, error) {
+


Will remove.

zedvijay · 2019-08-02T15:17:25Z

pkg/pillar/cmd/downloader/downloader.go

+	}
+
+	// XXX:FIXME - Why should this directory be created again
+	// when it was created in the first place in device-steps.sh


Yes, why should it be ?

zedvijay · 2019-08-02T15:18:10Z

pkg/pillar/cmd/downloader/downloader.go

+	rktAuth := types.RktAuthInfo{
+		RktKind:    "dockerAuth",
+		RktVersion: "v1",
+		Registries: []string{"registry-1.docker.io"},


registry-1, hardcoded?

Needs to be fixed. Need a way to translate URL into Registry name.. Will start a thread on this.

zedvijay · 2019-08-02T15:19:59Z

pkg/pillar/cmd/downloader/downloader.go

+		status.Size = 0
+		status.LastErr = fmt.Sprintf("%v", err)
+		status.LastErrTime = time.Now()
+		status.RetryCount += 1


status.State doesn't change here with download failed. How many time RetryCount is going to go up?

Right now there is no retry..

Currently, there is no DOWNLOAD_ERROR state. status.State is looked at only if there is no error.

zedvijay · 2019-08-02T15:20:42Z

pkg/pillar/cmd/downloader/downloader.go

@@ -1190,6 +1357,13 @@ func handleSyncOp(ctx *downloaderContext, key string,
 		log.Fatalf("handleSyncOp: No ObjType for %s\n",
 			status.Safename)
 	}
+
+	log.Debugf("handleSyncOp: config: %+v", config)


I think it implicitly adds a \n ( log.Debugf )

zedvijay · 2019-08-02T15:20:50Z

pkg/pillar/cmd/downloader/downloader.go

@@ -1190,6 +1357,13 @@ func handleSyncOp(ctx *downloaderContext, key string,
 		log.Fatalf("handleSyncOp: No ObjType for %s\n",
 			status.Safename)
 	}
+
+	log.Debugf("handleSyncOp: config: %+v", config)
+	log.Debugf("handleSyncOp: IsContainer: %v", config.IsContainer)


zedvijay · 2019-08-02T15:22:47Z

pkg/pillar/cmd/verifier/verifier.go

-		return
-	}
-	publishVerifyImageStatus(ctx, &status)
+		if !verifyObjectSha(ctx, config, &status) {


need to run go fmt

rvs · 2019-08-02T17:11:53Z

@spatchineelam @kalyan-nidumolu as part of this review you both should start going over Yetus feedback https://1561-182198941-gh.circle-artifacts.com/0/tmp/yetus-out/diff-patch-revive.txt

I'll provide more details later today

rvs · 2019-08-02T17:18:19Z

pkg/pillar/types/zedmanagertypes.go

+	IsContainer bool
+	// XXX:FIXME - Delete ContainerUrl atribute. rkt run will use
+	//      ContainerImageId instead.
+	ContainerUrl     string


I looked through the patch and it seems that ContainerUrl and URL is fairly trivial to remove now. Lets not keep this XXX:FIXME and do it before we merge the patch.

Taken care of in latest commit

rvs · 2019-08-02T17:20:13Z

pkg/pillar/types/domainmgrtypes.go

@@ -103,6 +108,10 @@ type DomainStatus struct {
 	LastErrTime        time.Time
 	BootFailed         bool
 	AdaptersFailed     bool
+	IsContainer        bool   // Is this Domain for a Container?
+	ContainerImageId   string // SHA-512 of rkt comtainer image
+	URL                string // rkt uses this URL to launch the container


I looked through the patch and it seems that ContainerUrl and URL is fairly trivial to remove now. Lets not keep this XXX:FIXME and do it before we merge the patch.

Taken care of in latest commit

rvs · 2019-08-02T17:22:44Z

pkg/pillar/types/domainmgrtypes.go

@@ -25,6 +26,10 @@ type DomainConfig struct {
 	VifList           []VifInfo
 	IoAdapterList     []IoAdapter
 	CloudInitUserData string // base64-encoded
+	// Container related info
+	IsContainer      bool   // Is this Domain for a Container?
+	URL              string // rkt uses this URL to launch the container


I looked through the patch and it seems that ContainerUrl and URL is fairly trivial to remove now. Lets not keep this XXX:FIXME and do it before we merge the patch.

We can remove ContainerUrl from DomainMgr types. We need that only between zedmanager and downloadmgr.

Taken care of in latest commit

rvs · 2019-08-02T17:23:57Z

pkg/pillar/scripts/device-steps.sh

+# Remove any old symlink to /var/lib/rkt
+rm -f /var/lib/rkt
+ln -s $PERSIST_RKT_DATA_DIR /var/lib/rkt
+


I believe we decided to remove this part @spatchineelam ?

Taken care of in latest commit

rvs · 2019-08-02T17:24:33Z

pkg/pillar/scripts/device-steps.sh

+DIRS="$CONFIGDIR $CONFIGDIR/DevicePortConfig"
+DIRS="$DIRS $TMPDIR $TMPDIR/DeviceNetworkConfig/ $TMPDIR/AssignableAdapters"
+DIRS="$DIRS $PERSISTDIR $PERSIST_RKT_DATA_DIR $PERSIST_RKT_CONF_LOCAL_DIR $PERSIST_RKT_CONF_LOCAL_AUTH_DIR"
+


Based on your research isn't it supposed to be moved to a different part of device-steps.sh @spatchineelam ?

Taken care of in latest commit

rvs · 2019-08-02T17:38:37Z

pkg/pillar/cmd/domainmgr/domainmgr.go

@@ -1860,6 +1877,87 @@ func handleDelete(ctx *domainContext, key string, status *types.DomainStatus) {
 		status.UUIDandVersion, status.DisplayName)
 }

+// Wrapper for domain creation thru xlCreate or rktRun
+func wrapDomainCreate(status *types.DomainStatus) (int, error) {


wouldn't a more natural name for this function simply be domainCreate? I mean containers ARE domains in our implementation.

Taken care of in latest commit

rvs · 2019-08-02T17:48:30Z

pkg/pillar/cmd/domainmgr/domainmgr.go

+}
+
+//  Launch app/container thru rkt
+// XXX:FIXME - check whether dom is launched in pause state


I've just committed a change into stage1 implementation that allows you to pass extra arguments to xl create. Lets fix this FIXME right now by passing STAGE1_XL_OPTS="-p"

Taken care of in latest commit

rvs · 2019-08-02T18:27:52Z

pkg/pillar/cmd/domainmgr/domainmgr.go

@@ -2022,6 +2120,56 @@ func xlUnpause(domainName string, domainId int) error {
 	return nil
 }

+// Wrapper for domain shutdown thru xlShutdown or rktStop
+func wrapDomainShutdown(status types.DomainStatus, force bool) error {


wouldn't a more natural name for this function simply be domainShutdown? I mean containers ARE domains in our implementation.

Taken care of in latest commit

rvs · 2019-08-02T18:28:43Z

pkg/pillar/cmd/domainmgr/domainmgr.go

+		// Use rkt tool
+		log.Infof("Using rkt tool ... PodUUID - %s\n", status.PodUUID)
+		err = rktStop(status.PodUUID, force)
+	} else {


I suggest you unconditionally follow with xlShutdown after rktStop

Taken care of in latest commit

rvs · 2019-08-02T18:29:41Z

pkg/pillar/cmd/domainmgr/domainmgr.go

@@ -2049,6 +2197,56 @@ func xlShutdown(domainName string, domainId int, force bool) error {
 	return nil
 }

+// Wrapper for domain Destroy thru xlDestroy or rktRm
+func wrapDomainDestroy(status types.DomainStatus) error {


wouldn't a more natural name for this function simply be domainDestroy? I mean containers ARE domains in our implementation.

Taken care of in latest commit

rvs · 2019-08-02T18:37:27Z

One last thing @spatchineelam and @kalyan-nidumolu -- please make sure to rebase on the latest EVE master before taking care of feedback @zedvijay and I provided. You'll need to be based off of https://github.com/lf-edge/eve/pull/170/files at least

kalyan-nidumolu

Some comments below.l

kalyan-nidumolu · 2019-08-05T08:41:44Z

pkg/pillar/cmd/domainmgr/domainmgr.go

+		var location string
+		if status.IsContainer {
+			// location := "/persist/rkt/cas/blob/sha512/" + "FIRST TWO CHARS IN SHA" + status.ContainerImageId
+			location = persistRktDataDir + "/cas/blob/sha512/" + string(status.ContainerImageId[:2]) + "/" + status.ContainerImageId


use golang path library: https://golang.org/pkg/path/#Join

Taken care of in latest commit

kalyan-nidumolu · 2019-08-05T08:41:55Z

pkg/pillar/cmd/domainmgr/domainmgr.go

+			// location := "/persist/rkt/cas/blob/sha512/" + "FIRST TWO CHARS IN SHA" + status.ContainerImageId
+			location = persistRktDataDir + "/cas/blob/sha512/" + string(status.ContainerImageId[:2]) + "/" + status.ContainerImageId
+		} else {
+			locationDir := verifiedDirname + "/" + dc.ImageSha256


Use golang path library

kalyan-nidumolu · 2019-08-05T08:50:14Z

pkg/pillar/cmd/domainmgr/domainmgr.go

+	uuidData, err := ioutil.ReadFile("/persist/rkt/uuid_file")
+	if err != nil {
+		log.Errorf("Open /persist/rkt/uuid_file failed : %s\n", err)
+		return 0, errors.New(fmt.Sprintf("open /persist/rkt/uuid_file failed: %s\n", err))


In case of this error, should re remove the container we launched? Otherwise, it is left dangling??

kalyan-nidumolu · 2019-08-05T08:52:48Z

pkg/pillar/cmd/domainmgr/domainmgr.go

+	status.PodUUID = strings.TrimSpace(string(uuidData))
+	log.Infof("PodUUID = %s\n", status.PodUUID)
+
+	// Temporary hack to obatin the domain id


Should we refactor this piece into a separate function and call from here?
getDomainIdForContainer()

or something like that? Keeps it more modular.

kalyan-nidumolu · 2019-08-05T08:55:21Z

pkg/pillar/cmd/domainmgr/domainmgr.go

+			status.DomainName, status.DomainId)
+		err = xlDestroy(status.DomainName, status.DomainId)
+		// wait for 3 seconds
+		time.Sleep(3)


Is 3 seconds good enough?? Please make this a descriptive constant instead of a magic number (3) ..

kalyan-nidumolu · 2019-08-05T09:03:02Z

pkg/pillar/cmd/downloader/downloader.go

+		status.Size = 0
+		status.LastErr = fmt.Sprintf("%v", err)
+		status.LastErrTime = time.Now()
+		status.RetryCount += 1


Right now there is no retry..

Currently, there is no DOWNLOAD_ERROR state. status.State is looked at only if there is no error.

kalyan-nidumolu · 2019-08-05T09:03:54Z

pkg/pillar/cmd/downloader/downloader.go

@@ -1190,6 +1357,13 @@ func handleSyncOp(ctx *downloaderContext, key string,
 		log.Fatalf("handleSyncOp: No ObjType for %s\n",
 			status.Safename)
 	}
+
+	log.Debugf("handleSyncOp: config: %+v", config)


I think it implicitly adds a \n ( log.Debugf )

kalyan-nidumolu · 2019-08-05T09:04:10Z

pkg/pillar/cmd/verifier/verifier.go

-		return
-	}
-	publishVerifyImageStatus(ctx, &status)
+		if !verifyObjectSha(ctx, config, &status) {


kalyan-nidumolu · 2019-08-05T09:05:45Z

pkg/pillar/cmd/zedmanager/handledomainmgr.go

-		if err != nil {
-			return err
+		location := ""
+		err := errors.New("")


It is not actually being used. I just need it declared. I think the right way is to declare it as
var err errors.Error
err = nil

kalyan-nidumolu · 2019-08-05T09:06:48Z

pkg/pillar/types/domainmgrtypes.go

@@ -25,6 +26,10 @@ type DomainConfig struct {
 	VifList           []VifInfo
 	IoAdapterList     []IoAdapter
 	CloudInitUserData string // base64-encoded
+	// Container related info
+	IsContainer      bool   // Is this Domain for a Container?
+	URL              string // rkt uses this URL to launch the container


We can remove ContainerUrl from DomainMgr types. We need that only between zedmanager and downloadmgr.

spatchineelam · 2019-08-06T18:13:05Z

I have squashed the whole lot of commits into 4 total commits, and applied Roman's CircleCI tweak on top.

Signed-off-by: Roman Shaposhnik <rvs@zededa.com>

First cut to publish Container ImageID in AppInstanceStatus Signed-off-by: Sankar Patchineelam <sankar@zededa.com>

First cut of changes for Domain Manager to support containers Domain manager changes to use external xl.cfg for stage1 xen config Taking care of Yetus warnings Signed-off-by: Sankar Patchineelam <sankar@zededa.com>

…nges for Domain Manager to support containers Domain manager changes to use external xl.cfg for stage1 xen config Taking care of Yetus warnings

pull the latest eve 3/30

spatchineelam requested review from eriknordmark and rvs as code owners August 1, 2019 15:33

zedvijay reviewed Aug 2, 2019

View reviewed changes

rvs requested changes Aug 2, 2019

View reviewed changes

kalyan-nidumolu reviewed Aug 5, 2019

View reviewed changes

spatchineelam force-pushed the cont_rkt_tip_aug_1st branch 2 times, most recently from 4dc8159 to ea68f74 Compare August 6, 2019 18:09

spatchineelam force-pushed the cont_rkt_tip_aug_1st branch from d4406d5 to c45113d Compare August 7, 2019 16:27

rvs and others added 3 commits August 9, 2019 09:07

Tweaking CircleCI

f900e3a

Signed-off-by: Roman Shaposhnik <rvs@zededa.com>

First cut of Container support for Downloader / Verifier

a169691

First cut to publish Container ImageID in AppInstanceStatus Signed-off-by: Sankar Patchineelam <sankar@zededa.com>

First cut for End to End Container App launch

973e4a5

First cut of changes for Domain Manager to support containers Domain manager changes to use external xl.cfg for stage1 xen config Taking care of Yetus warnings Signed-off-by: Sankar Patchineelam <sankar@zededa.com>

spatchineelam force-pushed the cont_rkt_tip_aug_1st branch from c45113d to 973e4a5 Compare August 9, 2019 04:54

kalyan-nidumolu added 4 commits August 9, 2019 12:07

fixup! First cut for End to End Container App launch First cut of cha…

cbabd0c

…nges for Domain Manager to support containers Domain manager changes to use external xl.cfg for stage1 xen config Taking care of Yetus warnings

fixup! First cut for End to End Container App launch First cut of cha…

99ed718

…nges for Domain Manager to support containers Domain manager changes to use external xl.cfg for stage1 xen config Taking care of Yetus warnings

fixup! First cut for End to End Container App launch First cut of cha…

35a9eb8

…nges for Domain Manager to support containers Domain manager changes to use external xl.cfg for stage1 xen config Taking care of Yetus warnings

fixup! First cut for End to End Container App launch First cut of cha…

830da18

…nges for Domain Manager to support containers Domain manager changes to use external xl.cfg for stage1 xen config Taking care of Yetus warnings

rvs closed this Aug 9, 2019

milan-zededa pushed a commit to milan-zededa/eve that referenced this pull request Nov 7, 2023

Merge pull request lf-edge#168 from lf-edge/master

1952446

pull the latest eve 3/30

		}

		func rktCreateAuthFile(config *types.DownloaderConfig) (string, error) {

Cont rkt tip aug 1st #168

Cont rkt tip aug 1st #168

Conversation

spatchineelam commented Aug 1, 2019

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

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

rvs commented Aug 2, 2019

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

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

rvs commented Aug 2, 2019

kalyan-nidumolu 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

spatchineelam commented Aug 6, 2019