If two artifacts provide the same file, re-link or re-copy that file from the artifact left after the other was undeployed/uninstalled. #3455
Conversation
…from the existing artifact after the other was undeployed/uninstalled.
mitchell-as
changed the title
|
Test failures are the usual timeouts and unrelated to this PR. |
|
This is a first attempt. I realize this PR may go through a lot of churn. |
pkg/runtime/depot.go
Outdated
| newSrc := filepath.Join(d.Path(artifactId), deployment.InstallDir, relativeDeployedFile) | ||
| logging.Debug("More than one artifact provides '%s'", relativeDeployedFile) | ||
| logging.Debug("Will relink %s to %s", deployedFile, newSrc) | ||
| relink[deployedFile] = newSrc |
There was a problem hiding this comment.
Should probably break here. What if we have three artifacts linking to the same file? We still want to address the first one encountered.
There was a problem hiding this comment.
I don't see the point of this. Deployment iteration is not ordered, so we cannot make any sort of guarantee which of 2 remaining artifacts will get picked.
Furthermore, we'd have to break out of two for loops, not one, so that would either require a goto or refactor to use functions. I don't think it's worth it since we cannot make the guarantee mentioned earlier.
Let me know if you feel strongly about this. "Should probably" didn't seem too strong to me :)
There was a problem hiding this comment.
Not being able to make a guarantee is already the reality we live in now. That is a problem for another day that we cannot address in this story. All we can address is that as long as there are artifacts still targeting a file, that file should exist. And one of them will arbitrarily win the race.
The main reason I brought this up is because as currently written we would remove and then link the file again here:
There was a problem hiding this comment.
Okay, I added a goto to break out of 3 for loops.
Also address PR feedback.
We no longer have to avoid removing files, which could have removed needed directories.
mitchell-as
force-pushed
the
mitchell/dx-2913
branch
from
e1137a2 to
fc84551
Compare
internal/smartlink/smartlink.go
Outdated
| if destDir := filepath.Dir(dest); !fileutils.DirExists(destDir) { | ||
| if err := os.MkdirAll(destDir, 0755); err != nil { | ||
| return errs.Wrap(err, "could not create directory %s", destDir) | ||
| } | ||
| } |
There was a problem hiding this comment.
Nit: You could simplify this with fileutils.MkdirUnlessExists().
There was a problem hiding this comment.
Okay. I also found a case where we can use fileutils.Mkdir() instead of os.Mkdir() with a magic number.
pkg/runtime/depot.go
Outdated
| @@ -355,10 +345,12 @@ func (d *depot) getSharedFilesToRedeploy(id strfmt.UUID, deploy deployment, path | |||
| logging.Debug("More than one artifact provides '%s'", relativeDeployedFile) | |||
| logging.Debug("Will redeploy '%s' to '%s'", newSrc, deployedFile) | |||
| redeploy[deployedFile] = newSrc | |||
| goto nextDeployedFile | |||
There was a problem hiding this comment.
Nit: Would you mind using an anonymous function or a var to break out of the outer loop?
This might be my own personal pet peeve, but I find the goto statement to be an eyesore. I've never encountered a scenario where this problem cannot be solved in a (in my opinion) cleaner way.
There was a problem hiding this comment.
Three consecutive
if fileFound {
break
}
looked hideous, so I tried an anonymous function. Hopefully that passes your filter :)
The idea behind this PR is to:
For example, given the following two packages:
state install hello, which provides a bin/main executable such thatstate exec mainprintsHello world!.state install hello-again, which provides a bin/main executable such thatstate exec mainprintsHello world again!.Here's a scenario:
state install hellostate install hello-againstate exec main--> printsHello world!fromhello, which "wins" by providing bin/main first.state uninstall hello--> identifies thathello-againalso provides bin/main, and re-links/re-deploys it after uninstallinghello.state exec main--> printsHello world again!state uninstall hello-again--> removes everything as expected.state exec main--> errors saying thatmainwas not found.Please note this is an "extreme" scenario. In all likelihood, artifacts providing the same file will provide the same bits, so the contents will not matter. The file merely needs to exist. Therefore, this PR effectively does reference counting and will not completely remove a file until no more artifacts reference it.