Skip to content
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

fix: handle more complex fields during merge #548

Merged
merged 19 commits into from
Jan 29, 2024

Conversation

glours
Copy link
Collaborator

@glours glours commented Jan 24, 2024

Improve the overall merge process for specific attribute format and uniqueness for lists and sequences

closes docker/compose#11417

@glours glours requested a review from ndeloof as a code owner January 24, 2024 22:02
@glours glours self-assigned this Jan 24, 2024
@glours glours requested a review from milas January 24, 2024 22:03
Copy link
Member

@milas milas left a comment

Choose a reason for hiding this comment

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

LGTM but note that we need the same for services.*.annotations, services.*.extra_hosts, and services.*.sysctls as well 😬

There might be others, but those are all k-v list or maps, so were easy to identify.

@glours glours marked this pull request as draft January 24, 2024 22:23
@logopk
Copy link

logopk commented Jan 25, 2024

I have noticed problems with duplicate IP subnets. docker-compose version 2.24.2 and 2.24.3

  default:
    driver: bridge
    ipam:
     config:
       - subnet: 172.18.0.0/29  

override:

  default:
    driver: bridge
    ipam:
     config:
       - subnet: 172.23.0.0/29

docker-compose config shows

  default:
    name: apache_default
    driver: bridge
    ipam:
      config:
        - subnet: 172.18.0.0/29
        - subnet: 172.23.0.0/29

Error message on up is:

 ✘ Network apache_default  Error                                                                                                                                  0.0s 
failed to create network apache_default: Error response from daemon: bridge driver doesn't support multiple subnets

This was not a problem so far. config shows the overridden subnet and up starts the container in this subnet

  default:
    name: apache_default
    driver: bridge
    ipam:
      config:
      - subnet: 172.23.0.0/29

if it would be the same subnet in both files it would not be merged:

  default:
    name: apache_default
    driver: bridge
    ipam:
      config:
        - subnet: 172.22.0.0/29
        - subnet: 172.22.0.0/29

leading to a more bizarre error:

 ✘ Network apache_default  Error                                                                                                                                  0.0s 
failed to create network apache_default: Error response from daemon: Pool overlaps with other one on this address space

@glours glours changed the title fix merge issue for labels Improve merge process Jan 25, 2024
@glours glours marked this pull request as ready for review January 26, 2024 17:15
@glours glours requested review from milas and ndeloof January 26, 2024 17:15
@@ -180,6 +195,16 @@ func mergeUlimit(_ any, o any, p tree.Path) (any, error) {
return o, nil
}

func mergeIPAMConfig(c any, o any, path tree.Path) (any, error) {
right := convertIntoMapping(c.([]any)[0], nil)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't get the logic here. Why only merge the first element ?
IIUC we should merge IPAM configs within the same network segment (same subnet) and append the others

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Rohh my bad 🤦

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'll fix that Monday 😞

network1:
ipam:
config:
- subnet: 172.28.0.0/16
Copy link
Collaborator

Choose a reason for hiding this comment

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

tes should include another subnet which should be kept unchanged, + another that should be appended, so we cover all use-cases

@milas milas changed the title Improve merge process fix: handle more complex fields during merge Jan 26, 2024
Copy link
Member

@milas milas left a comment

Choose a reason for hiding this comment

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

LGTM besides the already-commented on IPAM merge behavior

@glours glours force-pushed the fix-labels-merge branch 2 times, most recently from 438baad to 11e6b47 Compare January 29, 2024 10:51
@glours glours enabled auto-merge (rebase) January 29, 2024 10:52
@glours glours disabled auto-merge January 29, 2024 11:10
@glours glours enabled auto-merge (rebase) January 29, 2024 11:11
@glours glours disabled auto-merge January 29, 2024 11:11
glours and others added 2 commits January 29, 2024 13:50
Signed-off-by: Guillaume Lours <[email protected]>
Signed-off-by: Milas Bowman <[email protected]>
Signed-off-by: Guillaume Lours <[email protected]>
milas and others added 17 commits January 29, 2024 13:50
Signed-off-by: Milas Bowman <[email protected]>
Signed-off-by: Guillaume Lours <[email protected]>
Signed-off-by: Guillaume Lours <[email protected]>
Signed-off-by: Milas Bowman <[email protected]>
Signed-off-by: Guillaume Lours <[email protected]>
Signed-off-by: Milas Bowman <[email protected]>
Signed-off-by: Guillaume Lours <[email protected]>
Signed-off-by: Milas Bowman <[email protected]>
Signed-off-by: Guillaume Lours <[email protected]>
Signed-off-by: Milas Bowman <[email protected]>
Signed-off-by: Guillaume Lours <[email protected]>
Signed-off-by: Nicolas De Loof <[email protected]>
Signed-off-by: Guillaume Lours <[email protected]>
Signed-off-by: Guillaume Lours <[email protected]>
Signed-off-by: Guillaume Lours <[email protected]>
Signed-off-by: Guillaume Lours <[email protected]>
Signed-off-by: Guillaume Lours <[email protected]>
Signed-off-by: Guillaume Lours <[email protected]>
@glours glours merged commit 5a8e677 into compose-spec:main Jan 29, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] Overwriting labels no longer working on v2.24.2
4 participants