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
Merged

rework stack management #695

merged 5 commits into from
Apr 9, 2019

Conversation

errordeveloper
Copy link
Contributor

@errordeveloper errordeveloper 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 Compare April 3, 2019 15:47
@errordeveloper
Copy link
Contributor Author

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

@errordeveloper
Copy link
Contributor Author

Still need to rewrite the unit tests.

@errordeveloper errordeveloper force-pushed the rework-stack-management branch 2 times, most recently from 59858bf to cbcb386 Compare April 4, 2019 07:55
@errordeveloper
Copy link
Contributor Author

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 Compare April 5, 2019 16:27
@errordeveloper
Copy link
Contributor Author

@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 :)

@errordeveloper
Copy link
Contributor Author

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 Compare April 8, 2019 10:24
@errordeveloper errordeveloper marked this pull request as ready for review April 8, 2019 10:24
@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 Compare April 8, 2019 15:32
- 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 merged commit ca9f733 into master Apr 9, 2019
@errordeveloper errordeveloper deleted the rework-stack-management branch April 9, 2019 08:43
torredil pushed a commit to torredil/eksctl that referenced this pull request May 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants