feat(units): persist cloud container addresses and ports #18110
Conversation
wallyworld
force-pushed
the
k8s-container-addresses
branch
from
d249ace
to
0919d65
Compare
wallyworld
force-pushed
the
k8s-container-addresses
branch
6 times, most recently
from
2d0939f
to
b124115
Compare
wallyworld
force-pushed
the
k8s-container-addresses
branch
from
b124115
to
f8dd1c4
Compare
|
/build |
There was a problem hiding this comment.
Thanks @wallyworld,
Initial grok, think I need to refresh my head and go for another pass now I have it all down.
I am concerned that the unit name in the ddl should just be the number and not have the application. But that is a side bar conversation and not related to this PR.
| // cloud container. | ||
| Device: application.ContainerDevice{ | ||
| Name: fmt.Sprintf("placeholder for %q cloud container", in.UnitName), | ||
| DeviceTypeID: linklayerdevice.DeviceTypeUnknown, |
There was a problem hiding this comment.
I understand that we are porting code across into a new database model but this has me worried. We do we considered cloud device types 'unknown'. We explicitly understand exactly what we are dealing with this stage and there is no ambiguity.
Makes me feel like we are about to destroy our data model with bad enums that don't represent the fact. i.e is there code that exists somewhere that says all cloud devices types are the ones that are unknown?
There was a problem hiding this comment.
These are a placeholder and previous conversations agreed on this approach. As far as I am aware don't have visibility to the underlying link layer device like we do for vms. We want a consistent data model so need to fill in the blanks so to speak.
There was a problem hiding this comment.
I still think unknown should be changed here to a new type to represent cloud container link layer devices. We will never come back to this. The call is either we accept bad data into our model now and live with it forever or break out an enum to represent this case properly now so our future selves don't have to unpick what "unknown" means.
There was a problem hiding this comment.
Discussed f2f, this aligns with the existing Juju model (see link layer device types in core). We could change but need to do it holistically.
Adds state code to the application domain to persist cloud container address and ports. Drive by fix to use a string rather than a string pointer for unit names.
Includes extra doc, typo fixes, minor code tweaks.
| // cloud container. | ||
| Device: application.ContainerDevice{ | ||
| Name: fmt.Sprintf("placeholder for %q cloud container", in.UnitName), | ||
| DeviceTypeID: linklayerdevice.DeviceTypeUnknown, |
There was a problem hiding this comment.
I still think unknown should be changed here to a new type to represent cloud container link layer devices. We will never come back to this. The call is either we accept bad data into our model now and live with it forever or break out an enum to represent this case properly now so our future selves don't have to unpick what "unknown" means.
wallyworld
force-pushed
the
k8s-container-addresses
branch
from
6b77677
to
187f6fe
Compare
|
/merge |
| return nil | ||
| } | ||
|
|
||
| type ports []string |
There was a problem hiding this comment.
This should be in the types.go file.
| } | ||
| if args.CloudContainer != nil { | ||
| if err := st.upsertUnitCloudContainer(ctx, tx, unitName, nodeUUID.String(), args.CloudContainer); err != nil { | ||
| return errors.Annotatef(err, "creating cloud container row for unit %q", unitName) | ||
| if err := st.upsertUnitCloudContainer(ctx, tx, args.UnitName, nodeUUID.String(), args.CloudContainer); err != nil { |
There was a problem hiding this comment.
I wouldn't say it's fine. We came to an agreement that it's allowable. I am still of the view that tight RunAtomic usage minimises branching in state.
| ) | ||
| `, unit) | ||
| if err != nil { | ||
| return errors.Trace(err) |
There was a problem hiding this comment.
I thought we weren't going to do this any more.
Adds state code to the application domain to persist cloud container address and ports.
To save a cloud container address, a placeholder link layer device is created to tie the address to the container. Because it's a placeholder, it doesn't have a MAC address so this column is now optional.
Service addresses are made a separate struct to container addresses. We don't do anything yet with service addresses but they are different to container addresses.
Fixes a bug deleting a unit and enhances the test to detect the issue - dependent records are counted in the test.
Also includes code to support managing the various related enums - address scope, origin etc, device type etc.
Drive by fix to use a string rather than a string pointer for unit names. The unit name will end up being supplied by the service layer, either created from the k8s pod number or by using a sequence number for vm units.
QA steps
application domain unit tests
Links
Jira card: JUJU-6586