Skip to content
This repository has been archived by the owner on Jan 10, 2023. It is now read-only.

Remove Launchguard #132

Merged
merged 4 commits into from
May 31, 2018
Merged

Remove Launchguard #132

merged 4 commits into from
May 31, 2018

Conversation

sargun
Copy link
Contributor

@sargun sargun commented May 24, 2018

No description provided.

@codecov
Copy link

codecov bot commented May 25, 2018

Codecov Report

Merging #132 into master will decrease coverage by 2.47%.
The diff coverage is 7.96%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #132      +/-   ##
==========================================
- Coverage   35.81%   33.34%   -2.48%     
==========================================
  Files          65       61       -4     
  Lines        7399     7066     -333     
==========================================
- Hits         2650     2356     -294     
+ Misses       4445     4422      -23     
+ Partials      304      288      -16
Impacted Files Coverage Δ
fslocker/fslocker.go 65.48% <ø> (ø) ⬆️
executor/runtime/types/types.go 84.31% <ø> (ø) ⬆️
vpc/allocate/allocate_network.go 0% <0%> (ø) ⬆️
executor/runtime/docker/docker.go 50.47% <5.26%> (-1.3%) ⬇️
executor/runner/runner.go 62.23% <71.42%> (-3.09%) ⬇️
api/netflix/titus/agent.pb.go 25.87% <0%> (-1%) ⬇️

@coveralls
Copy link

coveralls commented May 26, 2018

Pull Request Test Coverage Report for Build 1187

  • 6 of 133 (4.51%) changed or added relevant lines in 3 files are covered.
  • 8 unchanged lines in 3 files lost coverage.
  • Overall coverage decreased (-2.8%) to 22.984%

Changes Missing Coverage Covered Lines Changed/Added Lines %
executor/runner/runner.go 6 8 75.0%
vpc/allocate/allocate_network.go 0 35 0.0%
executor/runtime/docker/docker.go 0 90 0.0%
Files with Coverage Reduction New Missed Lines %
executor/runner/runner.go 1 54.87%
vpc/allocate/allocate_network.go 2 0.0%
api/netflix/titus/agent.pb.go 5 17.55%
Totals Coverage Status
Change from base Build 1148: -2.8%
Covered Lines: 2058
Relevant Lines: 8954

💛 - Coveralls

@sargun sargun changed the title Add gox to ci builder image Remove Launchguard May 29, 2018
@@ -661,14 +671,29 @@ func prepareNetworkDriver(cfg Config, c *runtimeTypes.Container) error {
"--batch-size", strconv.Itoa(cfg.batchSize),
}

// This blocks, and ignores kills.
if !c.TitusInfo.GetIgnoreLaunchGuard() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If IgnoreLaunchGuard is true, why would we want to skip this step?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If IgnoreLaunchGuard is true, we should be using the kill initiated state, so we should never have two tasks using the same interface for different security groups.

if exitErr, ok := e.(*exec.ExitError); ok {
c.SetupCommandStatus <- exitErr
} else {
log.Error("Could not handle exit error of setup command: ", e)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we push e through the channel?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That was an error.

@@ -118,8 +120,9 @@ func (locker *FSLocker) ExclusiveLock(path string, timeout *time.Duration) (*Exc
}

// SharedLock tries to get an exclusive Lock on the path
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment should be s/exclusive/shared/

@@ -51,10 +51,15 @@ var AllocateNetwork = cli.Command{ // nolint: golint
Usage: "How long to wait for security groups to converge, in seconds",
Value: 10 * time.Second,
},
cli.DurationFlag{
Name: "wait-for-sg-lock-timeout",
Usage: "How long to wait for other users, if the SG is in use",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In what scenario would we now expect contention on the same SG? If the SG should be released prior to signaling master of completion, is this just waiting in unexpected/error scenarios?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This timeout will never get hit in the case of the same set of SGs. Only when changing SGs. It's basically, because v2 engine is going to stick around for the forseeable, and we need a solution for that. This is kind of like launchguard-lite.

@sargun sargun merged commit 594bc8a into master May 31, 2018
@sargun sargun deleted the remove-launchguard branch May 31, 2018 17:14
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants