Implement new Links format #1450
Conversation
steiler
force-pushed
the
newLinkInputFormat
branch
from
e6e3872
to
edda5f5
Compare
types/link.go
Outdated
| func briefLinkConversion(lc *LinkConfig) (*LinkConfig, error) { | ||
| // check two endpoints defined | ||
| if len(lc.Endpoints) != 2 { | ||
| return nil, fmt.Errorf("endpoint definition should consist of exactly 2 entries. %d provided", len(lc.Endpoints)) | ||
| } | ||
| for _, v := range lc.Endpoints { | ||
| parts := strings.SplitN(v, ":", 2) | ||
| node := parts[0] | ||
|
|
||
| lt, err := ParseLinkType(node) | ||
| if err != nil { | ||
| // if the link type parsing from the node name did fail | ||
| // we continue, since the node name is not like veth or macvlan or the like | ||
| continue | ||
| } | ||
|
|
||
| // if the node name is equal to a LinkType, we check the Type and only allow the depricated format | ||
| // for old types. New once we force to use the new link format. | ||
| switch lt { | ||
| case LinkTypeMgmtNet: | ||
| continue | ||
| case LinkTypeHost: | ||
| continue | ||
| case LinkTypeMacVLan, LinkTypeMacVTap: | ||
| return nil, fmt.Errorf("Link type %q needs to be defined in new link format", string(lt)) | ||
| } | ||
| } | ||
| return lc, nil | ||
| } |
There was a problem hiding this comment.
Hi @steiler
I am close to finialize the review, and I am concerned about this piece.
I think we should allow using the brief format for macvlan links, there is nothing wrong with it. I am quite positive that users will in 90% cases use the brief link format, as the new link format is too verbose to type in.
With that in mind my view is that internally we should use the links foundation you created here, but we need to strive to keep the brief format supported. Otherwise people will complain that it is too hard to define topologies with clab.
So I wonder if you are ok with removing this function entirely and allowing us to keep brief link definition type.
There was a problem hiding this comment.
Yes, possible.
It was now a means to restrict the brief format to existing links (switch statement) but is also meant to, in one of the the next step perform the conversion from the brief format, which is now represented by the LinkConfig to the type specific implementation. Where LinkConfig is just the raw / input version of the brief format.
There was a problem hiding this comment.
removed conversion in 1b9397b
we will add it when it will convert to concrete link types
types/link.go
Outdated
| // MarshalYAML serializes LinkDefinition (e.g when used with generate command). | ||
| // As of now it falls back to converting the LinkConfig into a | ||
| // RawVEthLink, such that the generated LinkConfigs adhere to the new LinkDefinition | ||
| // format instead of the brief one. | ||
| func (r *LinkDefinition) MarshalYAML() (interface{}, error) { | ||
| rawVEth, err := vEthFromLinkConfig(&r.LinkConfig) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| x := struct { | ||
| RawVEthLink `yaml:",inline"` | ||
| Type string `yaml:"type"` | ||
| }{ | ||
| RawVEthLink: *rawVEth, | ||
| Type: string(LinkTypeVEth), | ||
| } | ||
|
|
||
| return x, nil | ||
| } |
There was a problem hiding this comment.
@steiler I wonder, why do we currently need a custom marshaller, since (*CLab).Links contains LinkDefinitions that are not different from the old/current implementation?
I mean that when we do generate topologies I'd stick to the brief format (and not the new verbose one). In that case we wouldn't (as it seems) need any custom marshallers, and quite a few code blocks could be remove.
There was a problem hiding this comment.
If you ask we, I do not want to get rid of LinkDefinition in the internal code. It should be the Raw input format (see comment below) and be converted to the specific type with resolved string references (e.g. string id -> node reference).
Yes I did think of allowing the old / brief format only for the veth links which I assume is at least 90% of the cases. And force the new format onto people for all the other link types.
If you think this is the wrong path... fine, we can broaden the accepted brief definition types.
There was a problem hiding this comment.
in b8e4ecd I removed the custom unmarshaller for LinkDefinition, and made generate command to use brief format by default.
This removed a few funcs as a consequence.
There was a problem hiding this comment.
it seems you removed the marshaller not the unmarshaller
There was a problem hiding this comment.
Yeah, I meant to say marshaller 😅
hellt
force-pushed
the
newLinkInputFormat
branch
from
f2e3fd3
to
1b9397b
Compare
also made generate to use implicit brief format
Codecov Report
@@ Coverage Diff @@
## main #1450 +/- ##
==========================================
+ Coverage 20.68% 21.83% +1.14%
==========================================
Files 59 65 +6
Lines 6646 6809 +163
==========================================
+ Hits 1375 1487 +112
- Misses 5143 5191 +48
- Partials 128 131 +3
|
|
@steiler I have finilized my review, thank you for doing this work, it lays a good foundation I hope. |
|
Should be fine |
No description provided.