-
Notifications
You must be signed in to change notification settings - Fork 46
TRITON-2497 Static IP support for sdc-docker #158
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
bahamat
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to fix make check, but otherwise this looks good.
|
|
|
@teutat3s It's a bit confusing, because the target is Specifically, the following sub-checks fail:
|
|
@bahamat thanks for clarifying, I'll see what I can do to contribute and fix these failing sub-checks. |
|
@Smithx10 I tried addressing the |
Static addresses: fix make check
|
@bahamat EDIT: probably this is required before merging this? |
|
Ok, this is looking good so far. I'd like to see at least some integration tests created, and the test run to see the results. The following tests should be created:
In order to do this, you'll need an image built that includes TritonDataCenter/node-triton-tags#5. The easiest way to do that is to change package.json in this commit to point triton-tags at |
|
@bahamat should the image already be built or does it take a bit? |
|
@teutat3s I think automatic builds only happen when they come from joyent owned repos (otherwise it could be dangerous). I've kicked off a build so you should see it in a few minutes. |
|
Something seems to be broken in that image: EDIT: Maybe a sync with Click to expand |
a04acd3 to
5a0a6eb
Compare
|
@bahamat would you be so kind and trigger another build? Or do you have an idea what else could have gone wrong with that image? |
|
Build started. Should be ready in about 5 minutes from now. |
|
Same result. Could there be a step missing in the build process? It seems the EDIT: |
|
Bug now filed: https://smartos.org/bugview/TRITON-2497 |
danmcd
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see that #157 is the "issue" that is now TRITON-2497. I know you've had this in production for $YEARS, but I'll wanna make sure we get enough current-people to review this, AND to make sure it's documented properly as well.
package.json
Outdated
| "tape": "^4.4.0", | ||
| "trace-event": "1.2.0", | ||
| "triton-tags": "1.4.0", | ||
| "triton-tags": "git+https://github.com/Smithx10/node-triton-tags#ed3272116c846473c3fceee4c630dde1f40589f5", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I relabeled TritonDataCenter/node-triton-tags#5 to also be part of TRITON-2497
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Once v1.4.2 of node-triton-tags lands you can fix this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ALSO, like with node-triton-tags, change the "Author" to be "Triton Data Center (tritondatacenter.com)" (or "Edgecast Cloud (edgecast.io)").
|
Also, there's an upstream merge problem here that needs fixing, probably package.json. |
lib/backends/sdc/networks.js
Outdated
| payload.networks = [ {uuid: network.uuid, primary: true} ]; | ||
| payload.networks = | ||
| [ {ipv4_uuid: network.uuid, primary: true} ]; | ||
| if (container.NetworkingConfig.EndpointsConfig[networkMode] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's possible for us to land here without a NetworkingConfig. I think we may need to check for null's all the way down. Not sure how this is typically handled in node.
if (container.NetworkingConfig != null && container.NetworkingConfig.EndpointsConfig != null && container.NetworkingConfig.EndpointsConfig[networkMode] != undefined) {|
If you would like to reproduce the copy-right check, the command is: |
Have you worked with submodules before @Smithx10 ? I find it a PITA and it took be A While (TM) to get it so I ask only out of my own horrid experiences. |
I usually just vendor deps, but not really... I went into the submodule and git checkout'd the latest commit. This could be completely wrong. Let me know if that looks right or what to do instead. I'll need to squash / force push before merging so no biggie. |
test/integration/helpers.js
Outdated
| 'Name': '', | ||
| 'MaximumRetryCount': 0 | ||
| } | ||
| }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Check wants a copyright update here, too:
deps/eng/tools/check-copyright -q
error: test/integration/helpers.js: copyright year not updated to 2025: ' * Copyright 2022 MNX Cloud, Inc.'
|
The |
danmcd
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did we still want the Edgecast copyright added? |
It doesn't need to be Edgecast, just 2025. One file is missing Copyright 2025 Bruce Smith , the others either have none, or have one new @Smithx10 one already. |
danmcd
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WEIRD it needs the comma?!?
Anyway, re-upping, and hopefully this build I just kicked works.
Same error: Lemme look again. |
Okay, it's a kind of bug in eng's check-copyright. Would've been a similar problem in the old days if there was an old Joyent followed by a new non-MNX; except now it's old MNX and new non-Edgecast. Your other copyright changes aren't triggering because there's no MNX AND no Edgecast copyright. I'm attaching a commit you should add. |
danmcd
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Kicked off build again! :)
AND IT WORKED! Here are the build artifacts in Manta. Please re-test or test-anew with them? |
|
I think also we will need to bump the version, right? |
|
In order for folks to use this, sdc-vmapi will need node-triton-tags 1.5.0 |
Probably to 0.8.0 from 0.7.11. A cursory search of the TritonDataCenter repositories suggest nobody cites this package version anywhere else, so I hope this means knock-on effects are minimal. |
Needs package.json bump, and discussion about other affected components.
danmcd
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you.
| "name": "sdc-docker", | ||
| "version": "0.7.11", | ||
| "version": "0.8.0", | ||
| "author": "MNX Cloud (mnx.io)", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor nit: we usually update the author line to:
"author": "Edgecast Cloud (edgecast.io)",
|
Nice progress here and in |
|
Testing results for
Trying to provision a docker container with a single The new code currently seems to assume that An additional note is that currently both |
|
The above was reproduced in Triton with fabrics disabled: Trying to provision a docker container with a single external network in this environment fails with: P.S. for testing a docker endpoint with a self signed certificate, this works: or |
|
From what I understand currently is we'd like the behavior to be: Providing the same UUID for triton.network.public and EndPointConfig is illegal. We don't support 2 interfaces on the same network. Usage of "triton.network.public" requires a container with a published port. "The new code currently seems to assume that --network would be used only for a private fabric network": To have a single External Network you'd run: for example: This means the "public" labels are used to only define the "public" network. Did you try with --ip? @teutat3s when I tested prior running --network and --label triton.network.public of the same UUID we had 2 interfaces on the same network. I believe @danmcd didn't think the behavior should happen if we can avoid it. |
Addresses #157