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

[DCOS-15645] Change sed in installer to not be GNU specific #1571

Merged
merged 1 commit into from
May 24, 2017

Conversation

adamtheturtle
Copy link
Contributor

@adamtheturtle adamtheturtle commented May 19, 2017

High Level Description

My goal was to make DC/OS Docker runnable from my Mac as a development convenience.

This - sed syntax which does not work on macOS - was the one blocker in DC/OS.

Related Issues

https://jira.mesosphere.com/browse/DCOS-15645 - Run DC/OS Docker on macOS without Vagrant

Further context and CI

I originally wrote a way of overwriting sed with GNU sed on macOS (see https://github.com/dcos/dcos-docker/pull/36/files#diff-04c6e90faac2675aa89e2176d2eec7d8R33). This would require no changes to DC/OS but may have unintended consequences for the user.

I was asked by @karlkfi in review to create a cross-platform compatible change and to add to DC/OS a test which runs on a macOS builder. The suggested test would potentially spin up a DC/OS Docker cluster on macOS. My preference is to not do that because:

  • That adds a support burden if anything breaks on macOS or the test, when for now I intended it as a development nice-to-have.
  • That is work that wasn't scheduled for my team's work - I did this change in order to get my own work done more quickly.

I therefore submit this change and ask for feedback on whether we should:

  • Merge this (or some variant),
  • Merge the DC/OS Docker change,
  • Merge this but require a test - blocking this issue until macOS support is prioritised and scheduled
  • Say that we don't want this, at least for now

I would like DC/OS Docker macOS support, but also because since mentioning it on Slack, there has been a good response which shows that this is wanted.

Other solutions

See the high level description for options which do not involve modifying this file.

Other solutions in this file include:

  • Use gawk and require it to be installed on macOS with brew install gawk (handled in DC/OS Docker):
gawk '/^#EOF#/ { f = 1 } f' $0 | tail -n +2 | tar xv
  • Come up with a solution which works on both macOS's awk and GNU awk, or macOS's sed and GNU sed. I failed to achieve this but I am not an expert in either tool.
  • Check if gsed is available, and if so, use it instead of sed.

Testing

I have tested this by running the following command on CentOS (DC/OS Docker Vagrant VM) and macOS.

python2 -c "\
import sys;
with open(sys.argv[1], 'rb') as f:
	content = f.read()
	before, after = content.split('\n#EOF#\n', 1)
	sys.stdout.write(after)
" dcos_generate_config.sh | tar xv

On both platforms the command succeeds.
I await tests mainly to see if my indentation is alright but I'm interested in feedback as early as possible.

Checklist for all PR's

@d2iq-mergebot
Copy link
Collaborator

This repo has @mesosphere-mergebot integration. You can interact with the following commands.

@mesosphere-mergebot bump-ee  
@mesosphere-mergebot label [Work In Progress |Request For Comment |Ready For Review |Ship It |Holding] 
  • PR creators can apply one of [Ready For Review |Work In Progress]. Owners can apply any label.

@adamtheturtle
Copy link
Contributor Author

cc @karlkfi @gpaul @mhrabovcin (I'm not really sure who to ping)

@BenWhitehead - pinging because I've seen that you have opinions on related matters.

I'd ask that reviewers please read the PR description for context.

@karlkfi
Copy link
Contributor

karlkfi commented May 19, 2017

It appears that BSD awk doesn't have the extension necessary to read and write binary files on non-POSIX systems. However, you can do the script in GNU awk without the tail by moving the print to before the matching:

gawk '{ if (f == 1) print; if (/^#EOF#/) f = 1 }' $0 | tar xv

I also found a way to do it with BSD sed, which is probably the preferred solution:

sed '1,/^#EOF#$/d' $0 | tar xv

It appears that GNU sed interprets both 0 and 1 as the first line in a delete method, while BSD sed only accepts 1.

I have confirmed this command works on both mac and ubuntu.

@adamtheturtle
Copy link
Contributor Author

Thank you @karlkfi . Do you prefer that to Python? I'm easy but my preference is Python as, to me, it looks clearer and someone else has dealt with worrying about cross-platform issues.

Copy link
Contributor

@BenWhitehead BenWhitehead left a comment

Choose a reason for hiding this comment

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

I'm relatively certain switching to python2 here will prevent the dcos_generate_config.sh from being able to run on CoreOS boxes since it doesn't have python2 installed.

To even consider this change it will have to work on CoreOS, which I think leaves us with sed/awk as the only options.

(A change to fix the build error is going to merge in the next train, where I suspect we would actually see the install fail for the test builds that run on CoreOS)

@adamtheturtle
Copy link
Contributor Author

@benh I will go with @karlkfi 's suggestion of sed.

@karlkfi karlkfi self-requested a review May 19, 2017 22:47
@karlkfi
Copy link
Contributor

karlkfi commented May 19, 2017

please squash this to 1 commit

@karlkfi
Copy link
Contributor

karlkfi commented May 20, 2017

You can test this on an existing installer:

$ sed -E -e 'H;1h;$!d;x' -e "s/sed '0,/sed '1,/" dcos_generate_config.sh > dcos_generate_config-mac.sh
$ bash dcos_generate_config-mac.sh --version
Extracting image from this script and loading into docker daemon, this step can take a few minutes
x dcos-genconf.0ce03387884523f026-58fd0833ce81b6244f.tar
7cbcbac42c44: Loading layer [==================================================>]   5.05MB/5.05MB
a7a7a30e140f: Loading layer [==================================================>]  22.47MB/22.47MB
6b26d42e4247: Loading layer [==================================================>]  4.074MB/4.074MB
ff3c215c5e6d: Loading layer [==================================================>]  169.9MB/169.9MB
1de60c5eef71: Loading layer [==================================================>]   2.56kB/2.56kB
2c4fa20de7b5: Loading layer [==================================================>]   23.4MB/23.4MB
0348a99c5a4a: Loading layer [==================================================>]  604.8MB/604.8MB
1615426e7721: Loading layer [==================================================>]  4.096kB/4.096kB
5619eb92323e: Loading layer [==================================================>]  3.072kB/3.072kB
d5622be4eb04: Loading layer [==================================================>]  6.656kB/6.656kB
f0aa24e3465e: Loading layer [==================================================>]  1.536kB/1.536kB
Loaded image: mesosphere/dcos-genconf:0ce03387884523f026-58fd0833ce81b6244f
{
  "variant": "",
  "version": "1.9.0"
}

@karlkfi
Copy link
Contributor

karlkfi commented May 20, 2017

I don't understand why, but the teamcity/create-release-pr build is failing:

[15:49:11][Building package dcos-launch variant <default>] pkgpanda.build.BuildError: Validation error when fetching sources for package: Provided sha1 didn't match sha1 of downloaded file, corrupt download saved as /teamcity/work/19ad0dff41587ee6/packages/cache/packages/dcos-launch/pyinstaller/8b491def32d02cedc912198930225cddfab2e871.zip.corrupt. Provided: d98d271f9b70021417b34a059766131a0d73fdb4, Download file's sha1: 643734c396f677fe56c5e67cdd69d18f58431126, Url: https://github.com/pyinstaller/pyinstaller/archive/8b491def32d02cedc912198930225cddfab2e871.zip

@mellenburg
Copy link

Looks like the artifact got updated. The current train has an update for this and this will go back to working once that goes in. You can cherry-pick it to unblock for now 773227c

@adamtheturtle adamtheturtle force-pushed the cross-platform-sed-DCOS-15645 branch from 6b2dd14 to ac8b5d2 Compare May 22, 2017 10:44
@mhrabovcin
Copy link
Contributor

@mellenburg do we actually know why the sha1 hash has changed?

@adamtheturtle adamtheturtle force-pushed the cross-platform-sed-DCOS-15645 branch from ac8b5d2 to 58c7433 Compare May 22, 2017 15:01
@adamtheturtle adamtheturtle changed the title [DCOS-15645] Swap sed for Python in an installer command [DCOS-15645] Change sed in installer to not be GNU specific May 22, 2017
@adamtheturtle adamtheturtle force-pushed the cross-platform-sed-DCOS-15645 branch from 58c7433 to e387104 Compare May 22, 2017 15:22
@timcharper
Copy link
Contributor

sha1 change is blocking #1505 too

@branden
Copy link
Contributor

branden commented May 22, 2017

The dcos-launch source fix needs to be backported to 1.9 as well. https://github.com/dcos/dcos/blob/1.9.1/packages/dcos-launch/buildinfo.json#L11

@BenWhitehead
Copy link
Contributor

The fix for pyinstaller just merged to master, can you please rebase this PR off latest master?

@adamtheturtle adamtheturtle force-pushed the cross-platform-sed-DCOS-15645 branch from e387104 to cff3042 Compare May 22, 2017 23:05
@adamtheturtle
Copy link
Contributor Author

@BenWhitehead Thank you, I have done this just now.

@branden I imagine that that request would be welcome in JIRA or perhaps another PR.

@karlkfi
Copy link
Contributor

karlkfi commented May 23, 2017

kicked integration-test/upgrade-onprem and integration-test/aws-deploy. Looks like a timeout and a flakey test...

@karlkfi
Copy link
Contributor

karlkfi commented May 23, 2017

integration-test/upgrade-onprem failed again with the same (possibly flakey) error: test_upgrade_vpc.marathon_app_tasks_survive_upgrade

@adamtheturtle
Copy link
Contributor Author

@mesosphere-mergebot label Ready For Review

@orsenthil
Copy link
Contributor

@mesosphere-mergebot label ship-it

orsenthil pushed a commit to mesosphere/dcos that referenced this pull request May 23, 2017
@orsenthil orsenthil mentioned this pull request May 23, 2017
@spahl spahl merged commit cff3042 into dcos:master May 24, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
10 participants