Skip to content

Clean up & deprecate crashdump #3049

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

Merged
merged 5 commits into from
May 30, 2017
Merged

Clean up & deprecate crashdump #3049

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
3bdf567
xapi_crashdump.ml: remove dead code
gaborigloi May 22, 2017
7c46f1b
Xapi_crashdump: replace nothrow function with helper
gaborigloi May 22, 2017
3c38e84
Add Inverness release to the datamodel
gaborigloi May 25, 2017
7983e36
Datamodel: deprecate crashdump XenAPI class
gaborigloi May 26, 2017
caa15cd
Datamodel: add comment about how to declare new XS release
gaborigloi May 26, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 10 additions & 1 deletion ocaml/idl/datamodel.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,9 @@ let ely_release_schema_minor_vsn = 108
let falcon_release_schema_major_vsn = 5
let falcon_release_schema_minor_vsn = 120

let inverness_release_schema_major_vsn = 5
let inverness_release_schema_minor_vsn = 120

(* List of tech-preview releases. Fields in these releases are not guaranteed to be retained when
* upgrading to a full release. *)
let tech_preview_releases = [
Expand Down Expand Up @@ -229,6 +232,12 @@ let get_product_releases in_product_since =
| x::xs -> go_through_release_order xs
in go_through_release_order release_order

let inverness_release =
{ internal = get_product_releases rel_inverness
; opensource = get_oss_releases None
; internal_deprecated_since = None
}

let falcon_release =
{ internal = get_product_releases rel_falcon
; opensource=get_oss_releases None
Expand Down Expand Up @@ -6624,7 +6633,7 @@ let crashdump_destroy = call

(** A crashdump for a particular VM, stored in a particular VDI *)
let crashdump =
create_obj ~in_db:true ~in_product_since:rel_rio ~in_oss_since:None ~internal_deprecated_since:None ~persist:PersistEverything ~gen_constructor_destructor:false ~name:_crashdump ~descr:"A VM crashdump"
create_obj ~in_db:true ~in_product_since:rel_rio ~in_oss_since:None ~internal_deprecated_since:(Some rel_inverness) ~persist:PersistEverything ~gen_constructor_destructor:false ~name:_crashdump ~descr:"A VM crashdump"
~gen_events:true
~doccomments:[]
~messages_default_allowed_roles:_R_POOL_OP
Expand Down
9 changes: 9 additions & 0 deletions ocaml/idl/datamodel_types.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ let rel_indigo = "indigo"
let rel_dundee = "dundee"
let rel_ely = "ely"
let rel_falcon = "falcon"
let rel_inverness = "inverness"

type api_release = {
code_name: string option;
Expand All @@ -64,6 +65,8 @@ type api_release = {
branding: string;
}

(* When you add a new release, use the version number of the latest release,
and "Unreleased" for the branding, until the actual values are finalised. *)
let release_order_full = [{
code_name = Some rel_rio;
version_major = 1;
Expand Down Expand Up @@ -179,6 +182,12 @@ let release_order_full = [{
version_major = 2;
version_minor = 7;
branding = "XenServer 7.2";
}; {
code_name = Some rel_inverness;
Copy link
Contributor

Choose a reason for hiding this comment

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

What happens if you put None here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think None here is for things that do not have a codename, but Inverness does.

Copy link
Contributor Author

@gaborigloi gaborigloi May 26, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Another option is to simply not add Inverness to release_order_full yet, and add it later. But maybe that would confuse the SDK or something.

Copy link
Contributor

@mseri mseri May 26, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I think it's worth trying

Copy link

@kc284 kc284 May 26, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Please leave the same version numbers as falcon and use Unreleased for the branding. We'll bump it when we know the actual number.
This is basically the same we were doing before, it's just that now all these variables are in one place.

Copy link

Choose a reason for hiding this comment

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

Actually it's probably worth adding a comment at the beginning of this list with this tip for future reference.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kc284 How about a comment like this:
(* For future XenServer versions, use the version number of the latest release, and "Unreleased" for the branding. *)

Copy link

@kc284 kc284 May 26, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I'd rather make it more explicit and less xenserver-centric: "When you add a new release, use .... until the actual values are finalised".

(** TODO replace with the actual version numbers when Inverness is released *)
version_major = 2;
version_minor = 7;
branding = "Unreleased";
};
]

Expand Down
21 changes: 7 additions & 14 deletions ocaml/xapi/xapi_crashdump.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,22 +11,15 @@
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU Lesser General Public License for more details.
*)
exception Not_implemented

let nothrow f () = try f() with _ -> ()

let create ~__context ~vM ~vDI =
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we leave the create function here in case it's needed in the future?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They can still find it in the git history though, and @jonludlam said that this API class isn't really used anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've removed it because I would have to add a check to it, but realized it's unused, removing these function saves use time I think, because we don't have to consider them when we make changes.

let cdumpref = Ref.make() in
let uuid = Uuid.to_string (Uuid.make_uuid()) in
Db.Crashdump.create ~__context ~ref:cdumpref ~uuid ~vM ~vDI ~other_config:[];
cdumpref

let destroy ~__context ~self =
Copy link
Contributor

Choose a reason for hiding this comment

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

Where do we use this? Can't we just move the function in an helper module and get rid of this module altogether?

Copy link
Contributor Author

@gaborigloi gaborigloi May 24, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

This is a XenAPI call, crashdump.destroy: https://xapi-project.github.io/xen-api/classes/crashdump.html,
so I think it's best to stick to the existing conventions and keep this file.

Copy link
Contributor

@mseri mseri May 24, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Then let's deprecate the whole class! 🗑️
Also have a look at the use of host_crashdump but I think that cannot be deprecated yet.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, but I think we can still merge this PR, as we can't just delete the whole class and its associated code right away, but this is still a useful cleanup in the meantime?

Copy link
Contributor

Choose a reason for hiding this comment

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

Please add the deprecation of the Crashdump class to this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good, I'll add that!:)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mseri I think we first need to add the next release (inverness?) to the datamodel, before we can deprecate this class?

Copy link
Contributor

Choose a reason for hiding this comment

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

@robhoes knows

Copy link
Member

Choose a reason for hiding this comment

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

Yes.

Stdext.Pervasiveext.finally
(nothrow (fun ()->
let vdi = Db.Crashdump.get_VDI ~__context ~self in
Helpers.call_api_functions ~__context
(fun rpc session_id ->
Client.Client.VDI.destroy rpc session_id vdi)))
(Helpers.log_exn_continue
"destroying crashdump"
(fun ()->
let vdi = Db.Crashdump.get_VDI ~__context ~self in
Helpers.call_api_functions ~__context
(fun rpc session_id ->
Client.Client.VDI.destroy rpc session_id vdi)))
(fun ()->
Db.Crashdump.destroy ~__context ~self)