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

Jenkins CI: update containers tag to 2019-01-25 #11295

Closed
wants to merge 3 commits into from

Conversation

TSC21
Copy link
Member

@TSC21 TSC21 commented Jan 25, 2019

This brings also a fix on the ros-kinetic container by reverting back to Python2.7 and avoid problems with not found dependencies, as px4tools.

@TSC21 TSC21 requested review from dagar and lamping7 January 25, 2019 01:34
@TSC21 TSC21 self-assigned this Jan 25, 2019
@TSC21 TSC21 force-pushed the update/containers_2019-01-24 branch from 945be37 to 8e11d86 Compare January 25, 2019 01:45
@TSC21
Copy link
Member Author

TSC21 commented Jan 25, 2019

v4 HW seems to be down. @dagar

@TSC21
Copy link
Member Author

TSC21 commented Jan 25, 2019

@lamping7 seems like FW test also tends to fail here...

@TSC21
Copy link
Member Author

TSC21 commented Jan 25, 2019

The failures are awkwardly hard to find a pattern. Sometimes it fails to find px4tools in one side, then one reboots the build, then it passes on that test but fails on the other...

@lamping7
Copy link
Member

Are there two containers with the same 2019-01-24 tag... one from @mrivi earlier (with the issues) and now this one. So, physical machines aren't pulling the new one?

@TSC21
Copy link
Member Author

TSC21 commented Jan 25, 2019

Are there two containers with the same 2019-01-24 tag

There previous tag was replaced and it was rebuilt with the new PR.

So, physical machines aren't pulling the new one?

Well they should be. Are we able to clear build cache? Would it make any difference?

@lamping7
Copy link
Member

My understanding is that docker checks the tag or label of the image. If it has it, it just uses it. The container wouldn't be used across jobs, but the image would.

@TSC21
Copy link
Member Author

TSC21 commented Jan 25, 2019

Docker checks the tag and triggers a build for that tag. If you actually delete the tag and then create a tag with the same name, it will trigger a new build of the container, but that time with the latest Dockerfile. So I believe that in Docker Hub you already have the appropriate container built. Now it's a matter of Jenkins get the latest container.

@lamping7
Copy link
Member

Yes. That's my point. Jenkins physical machine possibly has the older version of the same tag.

@TSC21
Copy link
Member Author

TSC21 commented Jan 25, 2019

Yes. That's my point. Jenkins physical machine possibly has the older version of the same tag.

Ok so to make sure we guarantee the correct container on the machines, we can issue a new tag tomorrow and then update this PR.

@TSC21
Copy link
Member Author

TSC21 commented Jan 25, 2019

@lamping7
Copy link
Member

lamping7 commented Jan 25, 2019

Error:

ODOM: Ex: The tf tree is invalid because it contains a loop.

Hmmm.... I had a feeling we'd uncover other things with this move.

PS. Isn't it tomorrow where you are?

@TSC21
Copy link
Member Author

TSC21 commented Jan 25, 2019

ODOM: Ex: The tf tree is invalid because it contains a loop.

Hmmm.... I had a feeling we'd need to uncover other things with this move.

This does not influence. That's only related with ROS tfs. The tf gets fixed as soon as MAVROS starts receiving local position from the FCU.

Note: That's related with the odom plugin, which is the only plugin using static tf's to apply frame rotations.

@lamping7
Copy link
Member

lamping7 commented Jan 25, 2019

What are all these?

thrust_scaling parameter is set to zero.

It armed, then 10s later disarmed and never moved.

@TSC21
Copy link
Member Author

TSC21 commented Jan 25, 2019

https://github.com/mavlink/mavros/blob/master/mavros/src/plugins/setpoint_raw.cpp#L237

@TSC21
Copy link
Member Author

TSC21 commented Jan 25, 2019

@lamping7 the thurst_scaling has to be 1 here for this case I suppose. You are not using any custom px4_config.yaml right?

@lamping7
Copy link
Member

Nothing custom. We use MAVROS px4.launch -> normal px4_config. Sorry I'm not up to date.

@TSC21
Copy link
Member Author

TSC21 commented Jan 25, 2019

For better granularity and increased flexibility, in mavros_posix_sitl.launch, we can replace px4.launch with https://github.com/mavlink/mavros/blob/master/mavros/launch/node.launch, and pass it our own custom px4_config.yaml. That way we can better control what parameters we pass to MAVROS.

@lamping7
Copy link
Member

lamping7 commented Jan 25, 2019

I moved us away from that a while back when I did cleanup. The point was to not have to maintain it here and keep it simple for users trying to replicate this. I wasn't expecting a breaking change like that.

@TSC21
Copy link
Member Author

TSC21 commented Jan 25, 2019

I moved us away from that a while back when I did cleanup. The point was to not have to maintain it here and keep it simple. I wasn't expecting a breaking change like that.

It now expects that you send a value to be scaled inside the plugin using the thrust_scaling. Setting it to 1 may work if you are actually sending the values already between 0 and 1. Though, we still need to change it upstream I guess.

@TSC21
Copy link
Member Author

TSC21 commented Jan 25, 2019

@lamping7 the containers are now fixed. The problem now is on the MAVROS side. And we won't have a release soon regarding that. So should we look into an alternative for now to load custom px4_config.yaml or should we keep the tag behind for now?

Note: only fails for the attitude controller.

@TSC21 TSC21 changed the title Jenkins CI: update containers tag to 2019-01-24 Jenkins CI: update containers tag to 2019-01-25 Jan 25, 2019
@TSC21
Copy link
Member Author

TSC21 commented Jan 25, 2019

MAVROS fix: mavlink/mavros#1161. This should be brought in a minor release, but that's dependent on @vooon.

@TSC21
Copy link
Member Author

TSC21 commented Jan 25, 2019

@dagar any input on the HW build failure?

@lamping7
Copy link
Member

lamping7 commented Jan 25, 2019

@TSC21 We're sending a 0-1 value (a constant 0.7 actually) here.

As for updating the container: We don't need to keep them all back. We can use #11294. How do you feel about me incorporating the container tag change over there (just for .ci/Jenkinsfile-SITL_tests) instead of here? This will let us keep the tag back for just the attitude test for now and show that the PR over there works.

@TSC21
Copy link
Member Author

TSC21 commented Jan 25, 2019

As for updating the container: We don't need to keep them all back. We can use #11294. How do you feel about me incorporating the container tag change over there (just for .ci/Jenkinsfile-SITL_tests) instead of here? This will let us keep the tag back for just the attitude test for now and show that the PR over there works.

@lamping7 Sounds good. So let's push #11294 and then I will rebase this one against it so to update the rest of the tags.

@TSC21
Copy link
Member Author

TSC21 commented Jan 25, 2019

Also I merged mavlink/mavros#1161. This will get in in a further MAVROS release.

@dagar
Copy link
Member

dagar commented Jan 26, 2019

Also I merged mavlink/mavros#1161. This will get in in a further MAVROS release.

Should we do anything before that's available?

@lamping7
Copy link
Member

Why don't we just specify the version of mavros inside the dockerfile? We can choose the version before the breaking change was introduced.

@TSC21
Copy link
Member Author

TSC21 commented Jan 28, 2019

Superseded

@TSC21 TSC21 closed this Jan 28, 2019
@TSC21 TSC21 deleted the update/containers_2019-01-24 branch January 28, 2019 21:43
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.

3 participants