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

plugin/nomad: Persistence when installing Waypoint on Nomad #2282

Merged
merged 20 commits into from
Oct 6, 2021

Conversation

xiaolin-ninja
Copy link
Contributor

@xiaolin-ninja xiaolin-ninja commented Sep 10, 2021

This began as a feature request from #1348.
This should also resolve #2173.

Persistence with host volume is fully tested and functional.
Persistence with CSI volume only works with a static IP.

To-dos:
[X] resolve issue of Nomad not connecting to the public IP of the EC2 Instance.
[ X] documentation update: https://github.com/hashicorp/learn/pull/3967

Copy link
Member

@briancain briancain left a comment

Choose a reason for hiding this comment

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

Nice! Left a few requests, will test after but overall looks great! 😃

internal/serverinstall/nomad.go Outdated Show resolved Hide resolved
internal/serverinstall/nomad.go Outdated Show resolved Hide resolved
internal/serverinstall/nomad.go Show resolved Hide resolved
internal/serverinstall/nomad.go Show resolved Hide resolved
Copy link
Contributor

@evanphx evanphx left a comment

Choose a reason for hiding this comment

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

Just a couple minor things and then you're there!

Comment on lines 145 to 149
var (
// bytes
defaultCSIVolumeCapacityMin = int64(1073741824)
defaultCSIVolumeCapacityMax = int64(2147483648)
)
Copy link
Contributor

Choose a reason for hiding this comment

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

These should probably be const at the file level, since it looks like they're establishing our expected defaults, and we generally put our expected defaults as consts at the top of the file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oooh, yeah I didn't make them global because I thought they weren't used outside this method, but this just reminded me I forgot to put them as defaults in the flags

},
},
MountOptions: &api.CSIMountOptions{
FSType: "xfs",
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this need to be configurable?

internal/serverinstall/nomad.go Show resolved Hide resolved
waypointUserID := 100
waypointGroupID := 1000
preTask.Config = map[string]interface{}{
// TODO(xx): pin busybox image
Copy link
Contributor

Choose a reason for hiding this comment

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

The more I think about this, I think this might be one of those rare cases where it's better not to pin. It seems unlikely that this chown command wouldn't work in any past or future version of busybox, and we're unlikely to remember to update the tag on a regular basis.

Copy link
Member

Choose a reason for hiding this comment

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

That makes sense! If pulling latest becomes an issue down the road we can always add another flag to pin this specific image, but defaulting to pull latest for now seems fine 👍🏻

@@ -3,6 +3,8 @@ package serverinstall
import (
"context"
"fmt"
"google.golang.org/grpc/codes"
"google.golang.org/grpc/status"
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd put these in the middle import chunk

Copy link
Contributor

@izaaklauer izaaklauer left a comment

Choose a reason for hiding this comment

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

Looks good!

@xiaolin-ninja xiaolin-ninja merged commit e7b316f into main Oct 6, 2021
@xiaolin-ninja xiaolin-ninja deleted the plugin/nomad_persistence branch October 6, 2021 21:16
izaaklauer pushed a commit that referenced this pull request Oct 8, 2021
Persistence when installing waypoint on nomad
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
4 participants