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.

Sign up for GitHub

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

Jump to bottom

Use production image for k8s tests #9038

Merged
merged 2 commits into from
May 28, 2020
Merged

Use production image for k8s tests #9038

merged 2 commits into from
May 28, 2020

Conversation

dimberman
Copy link
Contributor

@dimberman dimberman commented May 27, 2020 •
edited
Loading

The CI image has become too large to load into KinD,

it also only really makes sense to use the production image for
integration tests

Make sure to mark the boxes below before creating PR: [x]

  • Description above provides context of the change
  • Unit tests coverage for changes (not needed for documentation changes)
  • Target Github ISSUE in description if exists
  • Commits follow "How to write a good git commit message"
  • Relevant documentation is updated including usage instructions.
  • I will engage committers as explained in Contribution Workflow Example.

In case of fundamental code change, Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in UPDATING.md.
Read the Pull Request Guidelines for more information.

@dimberman dimberman requested review from potiuk and ashb and removed request for potiuk May 27, 2020 16:17
@dimberman dimberman force-pushed the use-production-image-for-k8s-tests branch from 4e0c817 to 2fe07d9 Compare May 27, 2020 16:17
dimberman
dimberman commented May 27, 2020
View reviewed changes
scripts/ci/in_container/kubernetes/docker/rebuild_airflow_image.sh
--target="main" \
-f Dockerfile.ci . >> "${OUTPUT_LOG}"
echo
#export AIRFLOW_PROD_BASE_TAG="${DEFAULT_BRANCH}-python${PYTHON_MAJOR_MINOR_VERSION}"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@potiuk can you please point me to how I can initialize these env variables. I can't find it and there are a lot of bash scripts to navigate.

Copy link
Member

Choose a reason for hiding this comment

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

Those variablse are all set in the HOST and you need to pass them to the container. But I think this is not really needed . I just rebased #8265 which moves setting up kubernetes kind cluster and deploying production image to host rather than to CI image and I am going to finish it up this week, so I think it's better to do it this way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@potiuk I agree your way is better, but until this is merged the k8s won't pass because the image is so massive so we should figure out a hack for now

@dimberman dimberman force-pushed the use-production-image-for-k8s-tests branch 2 times, most recently from f0b26bd to 3edef37 Compare May 27, 2020 19:12
@dimberman dimberman requested a review from kaxil May 27, 2020 19:39
@turbaszek
Copy link
Member

Still the same, old error Couldn't resolve host 'github.com' :<

But it seems the persistent mode work. Would we be ok with having only one k8s test for a while?

@potiuk
Copy link
Member

potiuk commented May 28, 2020

I think this pinging git will be fixed with creating KinD on the host as per #8265 -> rebased it yesterday and plan to finish it this week.

@dimberman
Copy link
Contributor Author

@turbaszek @potiuk @ashb I'm honestly fine with turning off git-sync testing for a week. Unless a change DIRECTLY affects git syncing the tests should be in sync with eachother.

@astro-sql-decorator
Use production image for k8s tests
1b5d8b4 
The CI image has become too large to load into KinD,

it also only really makes sense to use the production image for
integration tests
@dimberman dimberman force-pushed the use-production-image-for-k8s-tests branch from c073c71 to 1b5d8b4 Compare May 28, 2020 13:02
@ashb
Copy link
Member

ashb commented May 28, 2020 •
edited
Loading

Do we even need to test git-sync to the level we do? That feels more like a system/integration test rather than something we need to check as part of every commit.

I.e. unit testing to make sure we drive/configure git-sync correctly should be Good Enough to run on every commit.

@dimberman
Copy link
Contributor Author

@ashb honestly I think that should be fine.

@ashb
Copy link
Member

ashb commented May 28, 2020

+1 to merging this now and fixing it later (or decided we don't need it on every commit, perhaps we only run git sync on nightly master builds?). We've been running without Kube tests for weeks and merging PRs anyway which is Not Good.

@dimberman
Copy link
Contributor Author

@ashb Yeah I REALLY want to get k8s testing turned on especially as I want to merge cherry-pick multiple k8s branches to 1.10

@dimberman dimberman merged commit e4d811d into apache:master May 28, 2020
@dimberman dimberman deleted the use-production-image-for-k8s-tests branch May 28, 2020 14:11
potiuk
potiuk approved these changes May 28, 2020
View reviewed changes
Copy link
Member

@potiuk potiuk left a comment

Choose a reason for hiding this comment

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

Sure. If you need it now - no problem, it's better than failing tests on Travis. I can rebase on top of that one and bring back git-sync if needed.

dimberman pushed a commit that referenced this pull request May 28, 2020
@astro-sql-decorator
Use production image for k8s tests (#9038)
77e6311 
    * Use production image for k8s tests

    The CI image has become too large to load into KinD,

    it also only really makes sense to use the production image for
    integration tests

    * nit

    Co-authored-by: Daniel Imberman <daniel@astronomer.io>
    (cherry picked from commit e4d811d)
dimberman pushed a commit that referenced this pull request May 28, 2020
@astro-sql-decorator
Use production image for k8s tests (#9038)
3a4e73c 
* Use production image for k8s tests

The CI image has become too large to load into KinD,

it also only really makes sense to use the production image for
integration tests

* nit

Co-authored-by: Daniel Imberman <daniel@astronomer.io>
(cherry picked from commit e4d811d)
dimberman pushed a commit that referenced this pull request May 29, 2020
@astro-sql-decorator
Use production image for k8s tests (#9038)
62e5696 
* Use production image for k8s tests

The CI image has become too large to load into KinD,

it also only really makes sense to use the production image for
integration tests

* nit

Co-authored-by: Daniel Imberman <daniel@astronomer.io>
(cherry picked from commit e4d811d)
dimberman pushed a commit that referenced this pull request May 29, 2020
@astro-sql-decorator
Use production image for k8s tests (#9038)
1e7616b 
* Use production image for k8s tests

The CI image has become too large to load into KinD,

it also only really makes sense to use the production image for
integration tests

* nit

Co-authored-by: Daniel Imberman <daniel@astronomer.io>
(cherry picked from commit e4d811d)
@potiuk potiuk added this to the Airflow 1.10.11 milestone Jun 5, 2020
kaxil pushed a commit that referenced this pull request Jul 1, 2020
@astro-sql-decorator @kaxil
Use production image for k8s tests (#9038)
23d5ea0 
* Use production image for k8s tests

The CI image has become too large to load into KinD,

it also only really makes sense to use the production image for
integration tests

* nit

Co-authored-by: Daniel Imberman <daniel@astronomer.io>
(cherry picked from commit e4d811d)
This was referenced Jul 14, 2020
Closed
Merged
kaxil added a commit to astronomer/airflow that referenced this pull request Jul 15, 2020
@kaxil
Change the Default run_as_user to 0 for K8s Workers
e56e1e8 
the default `run_as_user` for Airflow <= 1.10.10 was an empty string which defaulted to `0` but we changed it to `50000` in apache#9038 (and 1.10.11).

This PR reverts this behaviour and uses same default as Airflow <= 1.10.10
cfei18 pushed a commit to cfei18/incubator-airflow that referenced this pull request Mar 5, 2021
@astro-sql-decorator
Use production image for k8s tests (apache#9038)
a7163d3 
* Use production image for k8s tests

The CI image has become too large to load into KinD,

it also only really makes sense to use the production image for
integration tests

* nit

Co-authored-by: Daniel Imberman <daniel@astronomer.io>
(cherry picked from commit e4d811d)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@turbaszek turbaszek turbaszek approved these changes

@potiuk potiuk potiuk approved these changes

@ashb ashb Awaiting requested review from ashb

@kaxil kaxil Awaiting requested review from kaxil

Assignees
No one assigned
Labels
area:dev-tools
Projects
None yet
Milestone
Airflow 1.10.11
Development

Successfully merging this pull request may close these issues.
5 participants
@dimberman @turbaszek @potiuk @ashb @astro-sql-decorator