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

Use latest aardvark-dns binary #344

Merged
merged 2 commits into from
Jul 15, 2022
Merged

Conversation

baude
Copy link
Member

@baude baude commented Jul 14, 2022

Instead of using a packaged version of aardvark-dns, we grabbed the
latest main branch zip copy and use it instead.

Signed-off-by: Brent Baude [email protected]

@baude
Copy link
Member Author

baude commented Jul 14, 2022

this should fix #308

.cirrus.yml Outdated Show resolved Hide resolved
contrib/cirrus/setup.sh Outdated Show resolved Hide resolved
contrib/cirrus/setup.sh Outdated Show resolved Hide resolved
Copy link
Member

@cevich cevich left a comment

Choose a reason for hiding this comment

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

Generally looks good, there are a few little things to fix to make it more robust & readable.

@cevich
Copy link
Member

cevich commented Jul 14, 2022

To be clear: The new images I'm building are not required in this PR. I'm just referencing it in case someone wonders how/where/why in the future.

@flouthoc
Copy link
Collaborator

LGTM just waiting for unresolved comments and discussion above.

@Luap99
Copy link
Member

Luap99 commented Jul 15, 2022

/lgtm

Copy link
Member

@cevich cevich left a comment

Choose a reason for hiding this comment

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

If we leave AARDVARK_DNS_BRANCH undefined Cirrus will default to main which will silently do the wrong thing when a new release-branch is created in the future.

Since you'll need to re-run the tests again anyway, I've got newly built images with the netavark/aardvark-dns packages installed + binaries stripped out. It's up to you if you want to incorporate them here, or I'm fine if that happens in another PR:

  • GCE image ID is c5353795591864320
  • fedora-aws is ami-09e5978f141900855
  • fedora-netavark-aws-arm64 is ami-00d7ccde5eaacc50e
  • fedora-podman-aws-arm64 is ami-02da46d8fa51e5316

.cirrus.yml Show resolved Hide resolved
@cevich
Copy link
Member

cevich commented Jul 15, 2022

Sorry @baude these kinds of changes can be so finicky to get right. I do appreciate you're taking the initiative to open this PR and start the work. With the AARDVARK_DNS_BRANCH definition this is fully LGTM and ready to go in.

Updating the images is optional, and arguably risky. Totally fine if that happens in another PR.

@baude
Copy link
Member Author

baude commented Jul 15, 2022

/hold

Instead of using a packaged version of aardvark-dns, we grabbed the
latest main branch zip copy and use it instead.

Signed-off-by: Brent Baude <[email protected]>
Copy link
Member

@cevich cevich left a comment

Choose a reason for hiding this comment

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

/lgtm

@flouthoc
Copy link
Collaborator

flouthoc commented Jul 15, 2022

/hold @cevich I think build_cross is failing with new image

@flouthoc
Copy link
Collaborator

Is it missing containernetworking-plugins in new image ?

@cevich
Copy link
Member

cevich commented Jul 15, 2022

I think build_cross is failing with new image

This might actually be okay. @lsm5 is going to remove it entirely in #338

@Luap99
Copy link
Member

Luap99 commented Jul 15, 2022

No it is missing netavark which makes podman fall back to cni which then errors because cni is missing. I don't understand why we exclude the netavark rpms.
Either way the cross build does not need special networking, can we add --network host?

@cevich
Copy link
Member

cevich commented Jul 15, 2022

Either way the cross build does not need special networking, can we add --network host?

It's using the cross-rs tool to call podman, not sure if you can specify options like that. I say we just ignore the failure and manually merge, since @lsm5 PR can probably get merged really soon.

@flouthoc
Copy link
Collaborator

Either way the cross build does not need special networking, can we add --network host?

It's using the cross-rs tool to call podman, not sure if you can specify options like that. I say we just ignore the failure and manually merge, since @lsm5 PR can probably get merged really soon.

SGTM if others are cool with it.

@flouthoc
Copy link
Collaborator

/hold cancel
/lgtm

Copy link
Collaborator

@flouthoc flouthoc left a comment

Choose a reason for hiding this comment

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

LGTM

Looks like some one needs to do admin merge since one check is red.

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jul 15, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: baude, cevich, flouthoc

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:
  • OWNERS [baude,cevich,flouthoc]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@cevich
Copy link
Member

cevich commented Jul 15, 2022

Looks like some one needs to do admin merge since one check is red.

@lsm #338 is rebased on this PR, so I think if I merge that one it will take care of this one.

@flouthoc
Copy link
Collaborator

Cool lets wait for #338 then

@cevich
Copy link
Member

cevich commented Jul 15, 2022

Ugh, so 338 is going to need some more time. I've got an idea...

@openshift-ci openshift-ci bot removed the lgtm label Jul 15, 2022
@cevich
Copy link
Member

cevich commented Jul 15, 2022

/lgtm

@openshift-ci openshift-ci bot added the lgtm label Jul 15, 2022
@flouthoc
Copy link
Collaborator

@cevich Looks like total success depends on build_cross, we gotta remove that dependency as well.

@cevich
Copy link
Member

cevich commented Jul 15, 2022

Oof, isn't that what I just did? Hrmmm. What did I miss.

@cevich
Copy link
Member

cevich commented Jul 15, 2022

oh the artifact. crap.

@cevich
Copy link
Member

cevich commented Jul 15, 2022

right. aardvark CI needs that 🤣

A future commit will bring in native ARM64 builds.  Until then, some
adjustments are needed to handle the gutted netavark/aardvark RPMs
causing podman (needed indirectly by cross-rs) to fail due to no
networking.

Signed-off-by: Chris Evich <[email protected]>
@cevich
Copy link
Member

cevich commented Jul 15, 2022

Okay, so hopefully that will get it. Once again for the third-time 😖

@cevich
Copy link
Member

cevich commented Jul 15, 2022

Okay, that's going to work. Anyone: please feel free to merge this assuming it passes and looks good. I've got to get some lunch.

@flouthoc
Copy link
Collaborator

/lgtm
/approved

@openshift-ci openshift-ci bot added the lgtm label Jul 15, 2022
@openshift-ci openshift-ci bot merged commit f95b798 into containers:main Jul 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants