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

Feature: Source CRD (draft) #2095

Draft
wants to merge 11 commits into
base: main
Choose a base branch
from
Draft

Feature: Source CRD (draft) #2095

wants to merge 11 commits into from

Conversation

BenElferink
Copy link
Contributor

@BenElferink BenElferink commented Dec 29, 2024

Opened as draft, to sync with main branch and trigger workflows...

damemi and others added 3 commits December 28, 2024 14:02
This introduces the basic implementation for
instrumenting/uninstrumenting via `Source` objects
(#2037)

This does not remove current instrumentation with the
`odigos-instrumentation` label, and does not do any checks to avoid
conflicts etc. That work will follow. Right now this just drops in the
basic workflow alongside the current label approach.

Changes:

* Add a `SourceReconciler` to the `startlangdetection` Instrumentor
controller.
  * Parse workload from `Source` object
* If the `Source` is not being deleted, add 2 finalizers (one used for
`Source` deletion, and another used by the
`deleteinstrumentedapplication` controller), add workload labels to the
`Source` for lookup, and continue with flow of creating an
`InstrumentationConfig` by calling
`requestOdigletsToCalculateRuntimeDetails`
* If the `Source` is being deleted (determined by a `DeletionTimestamp`
being present in an Update event, with the finalizer blocking deletion),
remove the source finalizer and delete the `InstrumentationConfig`.
* Add a function `GetSourceListForWorkload` everywhere we currently call
`workload.IsWorkloadInstrumentationEffectiveEnabled` that returns any
`Source` objects referencing the workload.
* Add `SourceReconciler` to `deleteinstrumentedapplication` Instrumentor
controller
* When `Source` object is deleted, remove instrumented application
finalizer and continue with reconciling the workload, which should
trigger uninstrumentation based on the new checks for
`GetSourceListForWorkload` throughout

I'm not a fan of using 2 finalizers and worry that we could end up with
a deadlock/race. This is mostly due to InstrumentedApplication, and
deprecating InstrumentedApplication should simplify the work required
for this greatly.

Still needs:

* Namespace instrumentation and exclusion
* Switch `odigos-instrumentation` label to be shorthand for creating a
`Source` on that workload/namespace
* This includes removing current label checks with
`workload.IsWorkloadInstrumentationEffectiveEnabled`
* UI integration
* Webhook for validation (immutability) and defaulting (labels)
@BenElferink BenElferink marked this pull request as draft December 29, 2024 14:19
damemi and others added 8 commits December 29, 2024 19:31
Missed this when adding the test. Now it will actually run
This pull request includes various changes across multiple files,
focusing on updating configurations, refining comments, and enhancing
functionality. The most important changes include updating Go version
formatting in the build workflow, adding new annotations and constants,
and refining permissions and roles in the resource files.

### Configuration Updates:
*
[`.github/workflows/build.yaml`](diffhunk://#diff-d0777657fa3fd81d23aaf7273e58aee453b04e67882517900c56daeef9b3e4c1L47-R47):
Updated the Go version formatting from double quotes to single quotes in
multiple job steps.
[[1]](diffhunk://#diff-d0777657fa3fd81d23aaf7273e58aee453b04e67882517900c56daeef9b3e4c1L47-R47)
[[2]](diffhunk://#diff-d0777657fa3fd81d23aaf7273e58aee453b04e67882517900c56daeef9b3e4c1L122-R122)
[[3]](diffhunk://#diff-d0777657fa3fd81d23aaf7273e58aee453b04e67882517900c56daeef9b3e4c1L135-R135)
[[4]](diffhunk://#diff-d0777657fa3fd81d23aaf7273e58aee453b04e67882517900c56daeef9b3e4c1L147-R147)
[[5]](diffhunk://#diff-d0777657fa3fd81d23aaf7273e58aee453b04e67882517900c56daeef9b3e4c1L159-R159)

### Constants and Annotations:
*
[`common/consts/consts.go`](diffhunk://#diff-179a19de0058edabb08618c747a6bf22f25d8408fdeeeac6b888c9089c5c139fR18-R20):
Added new annotations for workload namespace, kind, and name, as well as
constants for CRD types.
[[1]](diffhunk://#diff-179a19de0058edabb08618c747a6bf22f25d8408fdeeeac6b888c9089c5c139fR18-R20)
[[2]](diffhunk://#diff-179a19de0058edabb08618c747a6bf22f25d8408fdeeeac6b888c9089c5c139fR32-R44)

### Permissions and Roles:
*
[`cli/cmd/resources/README.md`](diffhunk://#diff-7c3af7b93ef4c36c2e6a5b2866deb5b05c5cb4261ba09ff786c36a0385405bdcL8-R8):
Updated the permissions table for various components, including adding
new verbs and resources for the UI component.
[[1]](diffhunk://#diff-7c3af7b93ef4c36c2e6a5b2866deb5b05c5cb4261ba09ff786c36a0385405bdcL8-R8)
[[2]](diffhunk://#diff-7c3af7b93ef4c36c2e6a5b2866deb5b05c5cb4261ba09ff786c36a0385405bdcL34-R40)
[[3]](diffhunk://#diff-7c3af7b93ef4c36c2e6a5b2866deb5b05c5cb4261ba09ff786c36a0385405bdcL59-R66)
*
[`cli/cmd/resources/ui.go`](diffhunk://#diff-c286e10d34710a80a59127b2b7951e8a33d9b9554e47d2f2b827fd690f2e53abL153-R158):
Refined comments and resource definitions for roles, including CRUD
operations and watch permissions for Odigos entities.
[[1]](diffhunk://#diff-c286e10d34710a80a59127b2b7951e8a33d9b9554e47d2f2b827fd690f2e53abL153-R158)
[[2]](diffhunk://#diff-c286e10d34710a80a59127b2b7951e8a33d9b9554e47d2f2b827fd690f2e53abL168-R168)
[[3]](diffhunk://#diff-c286e10d34710a80a59127b2b7951e8a33d9b9554e47d2f2b827fd690f2e53abL217-R217)
[[4]](diffhunk://#diff-c286e10d34710a80a59127b2b7951e8a33d9b9554e47d2f2b827fd690f2e53abL233-R245)

### Code Refinements:
*
[`frontend/endpoints/sse/sse.go`](diffhunk://#diff-282d3dde003340e1665a5c9880de17fbb1f94a6add776b513c5bcadc82b9ec68L14-R32):
Added new message types and events for SSE messages.
*
[`frontend/graph/conversions.go`](diffhunk://#diff-019434fdf7b707424898bb20db99af0451a759ddf4678f82a3fbb2cee6bd3bd8L45-R48):
Modified function to map instrumentation configurations to actual
sources, including container runtime details.
[[1]](diffhunk://#diff-019434fdf7b707424898bb20db99af0451a759ddf4678f82a3fbb2cee6bd3bd8L45-R48)
[[2]](diffhunk://#diff-019434fdf7b707424898bb20db99af0451a759ddf4678f82a3fbb2cee6bd3bd8L62-L85)
*
[`frontend/graph/model/models_gen.go`](diffhunk://#diff-642ccd7ed71fdfa394bd7f7fd99c9c33e20ff18c876a91cb989d379a44390469L77-R78):
Updated `ComputePlatform` struct to include new fields for actual
sources and namespaces.

### Minor Changes:
*
[`Makefile`](diffhunk://#diff-76ed074a9305c04054cdebb9e9aad2d818052b07091de1f20cad0bbac34ffb52L202-R202):
Added `--nowait` flag to the `cli-install` target.
*
[`autoscaler/controllers/gateway/configmap.go`](diffhunk://#diff-5a57fdddd0c023b6580205ed7a5fb18405c69cc2861091d86fd1e6d692e634fbL153-R153):
Corrected a capitalization error in a log message.

These changes collectively improve the build process, enhance code
readability, and ensure proper configuration and permissions management
across the project.

---------

Co-authored-by: Mike Dame <mike@odigos.io>
Co-authored-by: Ron Federman <ron@keyval.dev>
This pull request includes multiple changes to improve the handling of
pending states and notifications in the frontend web application. The
most important changes include the addition of pending state management,
updates to notification handling, and minor UI improvements.

### Pending State Management:

*
[`frontend/webapp/hooks/destinations/useDestinationCRUD.ts`](diffhunk://#diff-c7f19ca063b62568e37726473e1b8c265d3309def0d157772951023c66e055b8R61-R71):
Added `addPendingItems` calls to manage pending states for create,
update, and delete destination actions.
*
[`frontend/webapp/hooks/sources/useSourceCRUD.ts`](diffhunk://#diff-1be86445dc0ed6bfdc08f7525c5800ef39bdcd70318a3c130f83f36f1ae9f5c8R37-R52):
Added `addPendingItems` calls to manage pending states for persisting
and updating sources.

### Notification Handling:

*
[`frontend/webapp/hooks/actions/useActionCRUD.ts`](diffhunk://#diff-97e103ce27156651b4a989f874cbddcdc9a3c2583c0a14e36d036bdd4f66933eL14-R15):
Updated to include `removeNotifications` in the notification store.
*
[`frontend/webapp/hooks/notification/useSSE.ts`](diffhunk://#diff-db9ebe0ce8cdabc0ede2f45de12661d55fb7aa50a5ecfc0787f5898c55b044d6L31-R39):
Added `setPendingItems` to clear pending states upon successful SSE
connection.

### UI Improvements:

*
[`frontend/webapp/containers/main/overview/overview-data-flow/build-destination-nodes.ts`](diffhunk://#diff-aa30a6b7893a9d1393f196b9846b1fdc19956c7c07f4099570cb25ff2f77fa43L76-R76):
Fixed a typo in the node title from 'ADD DESTIONATION' to 'ADD
DESTINATION'.
*
[`frontend/webapp/containers/main/overview/overview-drawer/index.tsx`](diffhunk://#diff-732721edf7c32d883ffefa52d6e43339c6ee328c88c3cb5f0d87f6760347b599R106-R123):
Added `useMemo` for pending state checks and integrated notification
handling for pending actions like edit and delete.
[[1]](diffhunk://#diff-732721edf7c32d883ffefa52d6e43339c6ee328c88c3cb5f0d87f6760347b599R106-R123)
[[2]](diffhunk://#diff-732721edf7c32d883ffefa52d6e43339c6ee328c88c3cb5f0d87f6760347b599L115-R137)

These changes enhance the application's user experience by providing
better feedback and handling for pending operations and notifications.

---------

Co-authored-by: Mike Dame <mike@odigos.io>
Co-authored-by: Ron Federman <ron@keyval.dev>
…s from API payload) (#2113)

This pull request includes several changes to improve error handling,
simplify TypeScript types, and update UI components in the frontend
codebase. The most important changes include modifications to error
handling in `services/sources.go`, simplification of TypeScript types
and payloads, and updates to UI components for better user experience.

Error Handling Improvements:
*
[`frontend/services/sources.go`](diffhunk://#diff-d8f62b6675961fc4e307e4fd622a59132ddc730b6e701ae1bc43dd1695f969a7L298-R298):
Modified `createSourceCRD` and `deleteSourceCRD` functions to return
errors instead of nil when a source is not found.
[[1]](diffhunk://#diff-d8f62b6675961fc4e307e4fd622a59132ddc730b6e701ae1bc43dd1695f969a7L298-R298)
[[2]](diffhunk://#diff-d8f62b6675961fc4e307e4fd622a59132ddc730b6e701ae1bc43dd1695f969a7L327-R326)

TypeScript Type Simplifications:
*
[`frontend/webapp/containers/main/destinations/add-destination/index.tsx`](diffhunk://#diff-a5ff632e01d3e161b70dce2f959db69fd1893edb484af12b7849810487347001L61-R62):
Simplified the payload handling by directly using `configuredSources`
and `configuredFutureApps`.
*
[`frontend/webapp/containers/main/sources/choose-sources/choose-source-modal/index.tsx`](diffhunk://#diff-2de6982f9ae22619e89eda6e002fd17bb887b0625bbd4281c7a778c352642979L18-R30):
Refactored to use `getApiPaylod` with forced types to satisfy TypeScript
without breaking the onboarding flow.

UI Component Updates:
*
[`frontend/webapp/containers/main/overview/multi-source-control/index.tsx`](diffhunk://#diff-cdd99a8fcd0484b24586f22b1f7b1bcf8d964bead53eeb0b2c07646c7301af70L53-R61):
Simplified the payload structure and ensured the deselect function is
called correctly.
*
[`frontend/webapp/containers/main/overview/overview-data-flow/build-action-nodes.ts`](diffhunk://#diff-2a2957e6035bd001e3c0c52f29f01f4473b6b99a8ce4381a08e9de504916c3b1L78-R78):
Updated subtitles for better clarity.
*
[`frontend/webapp/containers/main/sources/choose-sources/choose-sources-body/choose-sources-body-fast/sources-list/index.tsx`](diffhunk://#diff-6b67649d370d208941f4e5a78c0c9de2f5b9b65fd49e0ede8ef57066982450aaL2-R4):
Removed unused imports and refactored the component for better
readability and performance.
[[1]](diffhunk://#diff-6b67649d370d208941f4e5a78c0c9de2f5b9b65fd49e0ede8ef57066982450aaL2-R4)
[[2]](diffhunk://#diff-6b67649d370d208941f4e5a78c0c9de2f5b9b65fd49e0ede8ef57066982450aaL69-L78)
[[3]](diffhunk://#diff-6b67649d370d208941f4e5a78c0c9de2f5b9b65fd49e0ede8ef57066982450aaR82-R95)
[[4]](diffhunk://#diff-6b67649d370d208941f4e5a78c0c9de2f5b9b65fd49e0ede8ef57066982450aaL117-R127)
[[5]](diffhunk://#diff-6b67649d370d208941f4e5a78c0c9de2f5b9b65fd49e0ede8ef57066982450aaL155-R141)

---------

Co-authored-by: Mike Dame <mike@odigos.io>
Co-authored-by: Ron Federman <ron@keyval.dev>
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.

2 participants