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

rework stack management #695

Merged
merged 5 commits into from Apr 9, 2019

Conversation

@errordeveloper
Copy link
Member

commented Apr 2, 2019

Description

  • cater for dry-run
  • cleaner main functions for create and delete commands
  • more general abstraction for tasks
  • expose all common operations through functions
  • remove special purpose functions

Checklist

  • Code compiles correctly (i.e make build)
  • Added tests that cover your change (if possible)
  • All unit tests passing (i.e. make test)
  • All integration tests passing (i.e. make integration-test)
  • Added/modified documentation as required (such as the README.md, and examples directory)

@errordeveloper errordeveloper force-pushed the rework-stack-management branch 7 times, most recently from 3b490b0 to fc1ba94 Apr 3, 2019

@errordeveloper

This comment has been minimized.

Copy link
Member Author

commented Apr 3, 2019

@rndstr would you mind to take a look at this please?

@errordeveloper errordeveloper requested a review from rndstr Apr 3, 2019

@errordeveloper

This comment has been minimized.

Copy link
Member Author

commented Apr 3, 2019

Still need to rewrite the unit tests.

@errordeveloper errordeveloper force-pushed the rework-stack-management branch 2 times, most recently from 59858bf to cbcb386 Apr 4, 2019

@errordeveloper

This comment has been minimized.

Copy link
Member Author

commented Apr 4, 2019

I think there is a bug, like a possible race condition... I'll need to investigate, perhaps unit tests will show.

pkg/cfn/manager/tasks.go Outdated Show resolved Hide resolved
pkg/ctl/delete/cluster.go Show resolved Hide resolved
pkg/cfn/manager/tasks.go Show resolved Hide resolved
pkg/cfn/manager/delete_tasks.go Outdated Show resolved Hide resolved
pkg/cfn/manager/deprecated.go Outdated Show resolved Hide resolved
pkg/cfn/manager/tasks.go Outdated Show resolved Hide resolved

@errordeveloper errordeveloper force-pushed the rework-stack-management branch 4 times, most recently from f84c72f to 44bc37e Apr 5, 2019

@errordeveloper

This comment has been minimized.

Copy link
Member Author

commented Apr 5, 2019

@martina-if thanks for spotting the race condition! As I mentioned, I was able to reproduce it with unit tests. I've fixed it a we have quite a few tests now. This PR is in a pretty good shape to merge next week :)

pkg/cfn/manager/tasks.go Outdated Show resolved Hide resolved

@errordeveloper errordeveloper force-pushed the rework-stack-management branch from 44bc37e to 090b122 Apr 8, 2019

@errordeveloper

This comment has been minimized.

Copy link
Member Author

commented Apr 8, 2019

Looks like we cannot use go test -race in alpine container due to CGO/musl:

# runtime/race
/usr/lib/gcc/x86_64-alpine-linux-musl/8.2.0/../../../../x86_64-alpine-linux-musl/bin/ld: race_linux_amd64.syso: in function `__sanitizer::GetArgv()':
gotsan.cc:(.text+0x4183): undefined reference to `__libc_stack_end'
/usr/lib/gcc/x86_64-alpine-linux-musl/8.2.0/../../../../x86_64-alpine-linux-musl/bin/ld: race_linux_amd64.syso: in function `__sanitizer::ReExec()':
gotsan.cc:(.text+0x9797): undefined reference to `__libc_stack_end'
/usr/lib/gcc/x86_64-alpine-linux-musl/8.2.0/../../../../x86_64-alpine-linux-musl/bin/ld: race_linux_amd64.syso: in function `__sanitizer::InternalAlloc(unsigned long, __sanitizer::SizeClassAllocatorLocalCache<__sanitizer::SizeClassAllocator32<__sanitizer::AP32> >*, unsigned long)':
gotsan.cc:(.text+0xaac1): undefined reference to `__libc_malloc'
/usr/lib/gcc/x86_64-alpine-linux-musl/8.2.0/../../../../x86_64-alpine-linux-musl/bin/ld: race_linux_amd64.syso: in function `__sanitizer::InternalRealloc(void*, unsigned long, __sanitizer::SizeClassAllocatorLocalCache<__sanitizer::SizeClassAllocator32<__sanitizer::AP32> >*)':
gotsan.cc:(.text+0xca20): undefined reference to `__libc_realloc'
/usr/lib/gcc/x86_64-alpine-linux-musl/8.2.0/../../../../x86_64-alpine-linux-musl/bin/ld: race_linux_amd64.syso: in function `__sanitizer::InternalFree(void*, __sanitizer::SizeClassAllocatorLocalCache<__sanitizer::SizeClassAllocator32<__sanitizer::AP32> >*)':
gotsan.cc:(.text+0x66e8): undefined reference to `__libc_free'
collect2: error: ld returned 1 exit status

@errordeveloper errordeveloper force-pushed the rework-stack-management branch 4 times, most recently from e7b5aca to 7f33b11 Apr 8, 2019

@errordeveloper errordeveloper marked this pull request as ready for review Apr 8, 2019

@errordeveloper errordeveloper changed the title WIP: rework stack management rework stack management Apr 8, 2019

pkg/cfn/manager/api.go Outdated Show resolved Hide resolved
pkg/cfn/manager/tasks.go Outdated Show resolved Hide resolved
pkg/cfn/manager/tasks.go Show resolved Hide resolved

@errordeveloper errordeveloper force-pushed the rework-stack-management branch 2 times, most recently from 050384a to 86a94f0 Apr 8, 2019

Rework stack management
- more visibility into what task manager does
  - print clear descriptions of what is being done
  - enable addition of `--dry-run` to every command
- cleaner main functions for create and delete commands
- more general abstraction for tasks
- expose all common operations through functions
- remove special purpose taks handler functions

@errordeveloper errordeveloper force-pushed the rework-stack-management branch from 86a94f0 to cac01aa Apr 9, 2019

@errordeveloper errordeveloper force-pushed the rework-stack-management branch from cac01aa to 6a10163 Apr 9, 2019

@errordeveloper errordeveloper requested a review from martina-if Apr 9, 2019

@errordeveloper errordeveloper merged commit ca9f733 into master Apr 9, 2019

2 checks passed

WIP Ready for review
Details
ci/circleci: make-eksctl-image Your tests passed on CircleCI!
Details

@errordeveloper errordeveloper deleted the rework-stack-management branch Apr 9, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.